Be consistent checking c standard in assert.h (#527)

[wanders]

Assert header is split into two to handle the idempotency since commit b3260c2 "17491 assert(3c) prototype visibility". The macro is in one and the prototype for the internal function in the other. There are different variants for pre-c99 and c99 and the macro and prototype needs to match. The aforementioned commit changed the prototype to use _STDC_C99 while the macro is using __STDC_VERSION__.

Normally _STDC_C99 and __STDC_VERSION__ would match and this wouldn't be a problem. As sys/feature_tests.h sets _STD_C99 based on __STDC_VERSION__ value.

However the gcc10 compiler is patched to set _STD_C99 as a predefined macro for C++ code.
TritonDataCenter/pkgsrc-extra@e295cdb

This caused using assert from c++ code to fail because the macro expanded a function that there was no proto type for.

[danmcd]

This should be an illumos-gate bug. I'll build SmartOS with this, but I'm worried that while it fixes your case, it might regress something somewhere else. I'm sppinning builds for overnight (all four of {default,debug} x {gcc10,gcc14}). If they build, I'd recommend skipping here and getting this straight into illumos-gate, with more eyes, including the folks behind illumos#17491, looking at it.

If you've never file a bug for -gate, it's not like here where you can PR. I'll help you with the process if we need to go that way.

[rmustacc]

@wanders the way I read the pkgsrc patch is that it sets the corresponding _STDC_VERSION to something that is at least C99. So what value of __STDC_VERSION__ are you seeing coming through in the actual failure case? Note, <sys/feature_tests.h> should already be present due to the include of <iso/assert_iso.h> I think.

[jperkin]

@rmustacc it's the opposite, I remove __STDC_VERSION__ when compiling in C++ mode. This makes my compiler match other platforms (Solaris/illumos are the only platforms that I'm aware of that do this) and fixes some third-party software that makes the (questionable) assumption that if __STDC_VERSION__ is defined then the code must be C, not C++.

I wasn't aware of any fallout from this (my bulk builds only showed positive improvements), so if this change is acceptable to illumos I'd like to also see the main illumos gccs also integrate this patch.

[wanders]

If they build, I'd recommend skipping here and getting this straight into illumos-gate, with more eyes, including the folks behind illumos#17491, looking at it.
If you've never file a bug for -gate, it's not like here where you can PR. I'll help you with the process if we need to go that way.

I did a "make live" in smartos-live with this patch and it built. And then did make live inside a system running the produced image. Not sure that is a comprehensive test, or even valid..

I've never filed a bug there (I just started playing around with illumos/smartos last week), so I'd appreciate any help there. The process seems to me a bit more involved than just throwing up an PR like here, which is why I started here.

@wanders the way I read the pkgsrc patch is that it sets the corresponding _STDC_VERSION to something that is at least C99. So what value of __STDC_VERSION__ are you seeing coming through in the actual failure case?

I might be missing something (reading diffs of patches always is confusing), but as I read it the commit TritonDataCenter/pkgsrc-extra@e295cdb removes _STDC_VERSION from the patch, and instead add _STDC_C11

-	    builtin_define ("__STDC_VERSION__=199901L");\
+	    builtin_define ("_STDC_C99");		\

Which matches the commit message "Switch to _STDC_C99/_STDC_C11"

I had more details in the corresponding github issue: #527

The predefined macros there are:

# C
$ gcc -x c -std=c99 -dM -E - </dev/null | grep 'STDC_[CV]'
#define __STDC_VERSION__ 199901L

# C++
$ gcc -x c++ -std=c++11 -dM -E - </dev/null | grep 'STDC_[CV]'
#define _STDC_C99 1

$ gcc -v 2>&1 | tail -n1
gcc version 13.3.0 (GCC) 

Note, <sys/feature_tests.h> should already be present due to the include of <iso/assert_iso.h> I think.

Yes, I added that include because I'm used to making sure each header includes what that header needs, not assuming things are pulled in by other. Maybe that is not practise here?

Maybe the proper fix is to not use _STD_C99 at all in assert(_iso).h. And instead do

#if (__cplusplus - 0 >= 201103L) || (__STDC_VERSION__ - 0) >= 199901L

[wanders]

I wasn't aware of any fallout from this (my bulk builds only showed positive improvements), so if this change is acceptable to illumos I'd like to also see the main illumos gccs also integrate this patch.

I guess all pkgsrc builds all packages in "release mode", i.e with NDEBUG defined such that the assert macro expands to nothing (or (void)0) to be exact).

To trigger the issue one must build without NDEBUG set.

[rmustacc]

Thanks for the follow up here, @wanders and @jperkin. Apologies for misreading the patch and missing #527. I can definitely agree that what we have here is inconsistent and problematic.

@wanders How did you pick C++11 as the defining point for the cutover. I'm having trouble determining when C++ nominally had support for the #EX bit we use in assert.h, but a quick test to me shows that #EX that we use is valid in C++98 and at least from https://en.cppreference.com/w/cpp/preprocessor/replace.html, it's only #__VAR_ARGS__ that is C++11-specific. But I will admit I do not follow C++ very much hence creating this bug in the first place!

I wasn't aware of any fallout from this (my bulk builds only showed positive improvements), so if this change is acceptable to illumos I'd like to also see the main illumos gccs also integrate this patch.

Well, our other bulk build testing was with something without the patch, though it also doesn't set NDEBUG on its own, unless the software builds themselves override it, so that's partially how we missed it.

I'm pretty uncomfortable with defining the internal macros like this from the compiler as that's only going to create more flag days, but I definitely get the context and desire here. I can definitely see how it causes software problems to declare a __STDC_VERSION__. A few observations here:

• It seems like if we're relying on language features, we should probably check that's C++ explicitly rather than assuming one way or the other.
• Alternatively if we want to actually say that this is actually true or have always been doing the equivalent in practice, should we be doing this instead in <sys/feature_tests.h> more generally? It's not clear to me that these things are equivalent. There are both namespace visibility and language features. As I've not tracked C++ standards, is there something that says the namespace visibility is inherited in some way like that described? At which point it's mostly us having to look for keyword and odder pre-processor usage (which is fairly minimal in our headers) that is disjoint if we think we should generally enable this.

I think what I'm tempted to put together is something that says we should just always use the C99 version in all C++ code based on the per-processor support assuming my test is correct. That should be fairly low risk and then we come back and address the compiler inconsistencies and how we should be taking a C++ version and translating it into features. In an ideal world that might let us drop that portion of the patches entirely.

[wanders]

Thanks for the follow up here, @wanders and @jperkin. Apologies for misreading the patch and missing #527. I can definitely agree that what we have here is inconsistent and problematic.

@wanders How did you pick C++11 as the defining point for the cutover. I'm having trouble determining when C++ nominally had support for the #EX bit we use in assert.h, but a quick test to me shows that #EX that we use is valid in C++98 and at least from https://en.cppreference.com/w/cpp/preprocessor/replace.html, it's only #__VAR_ARGS__ that is C++11-specific. But I will admit I do not follow C++ very much hence creating this bug in the first place!

As I understand it the c99 check is there for __func__ which was added in c99. Not the stringizing operator, which has been around before standardisation I think.

In pre-c99 the assert macro expands to __assert which doesn't take function name, while in c99 it expands to __assert_c99 which takes function name.

c++11 introduced __func__ (defined in 8.4.1:8 on page 188 in the [https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf](c++11 draft).
Here is the paper proposing it: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1642.html

I wasn't aware of any fallout from this (my bulk builds only showed positive improvements), so if this change is acceptable to illumos I'd like to also see the main illumos gccs also integrate this patch.

Well, our other bulk build testing was with something without the patch, though it also doesn't set NDEBUG on its own, unless the software builds themselves override it, so that's partially how we missed it.

I'm pretty uncomfortable with defining the internal macros like this from the compiler as that's only going to create more flag days, but I definitely get the context and desire here. I can definitely see how it causes software problems to declare a __STDC_VERSION__. A few observations here:

• It seems like if we're relying on language features, we should probably check that's C++ explicitly rather than assuming one way or the other.

• Alternatively if we want to actually say that this is actually true or have always been doing the equivalent in practice, should we be doing this instead in <sys/feature_tests.h> more generally? It's not clear to me that these things are equivalent. There are both namespace visibility and language features. As I've not tracked C++ standards, is there something that says the namespace visibility is inherited in some way like that described? At which point it's mostly us having to look for keyword and odder pre-processor usage (which is fairly minimal in our headers) that is disjoint if we think we should generally enable this.

I think what I'm tempted to put together is something that says we should just always use the C99 version in all C++ code based on the per-processor support assuming my test is correct. That should be fairly low risk and then we come back and address the compiler inconsistencies and how we should be taking a C++ version and translating it into features. In an ideal world that might let us drop that portion of the patches entirely.

I can't really comment on whether the compiler should predefine the _STDC_Cnn or not as I don't have the history and understanding of the legacy.

And I have not tried to investigate what the headers use _STDC_Cnn defines for.

But from my limited perspective it would seem like it would be better to set them from headers, rather than magically in compiler, like:

--- a/usr/src/uts/common/sys/feature_tests.h
+++ b/usr/src/uts/common/sys/feature_tests.h
@@ -165,11 +165,11 @@ extern "C" {
 #define        _STDC_C17
 #endif
 
-#if __STDC_VERSION__ - 0 >= 201112L
+#if (__cplusplus - 0 >= 201703L) || (__STDC_VERSION__ - 0 >= 201112L)
 #define        _STDC_C11
 #endif
 
-#if __STDC_VERSION__ - 0 >= 199901L
+#if (__cplusplus - 0 >= 201103L) || (__STDC_VERSION__ - 0 >= 199901L)
 #define        _STDC_C99
 #endif
 

The _STDC_C99 macro is documented as only for internal use in system headers, so doesn't seem clean that the compiler is messing with them.
[usr/src/uts/common/sys/feature_tests.h]

 * The feature test macros __XOPEN_OR_POSIX, _STRICT_STDC, _STRICT_SYMBOLS,
 * and _STDC_C99 are Sun implementation specific macros created in order to
 * compress common standards specified feature test macros for easier reading.
 * These macros should not be used by the application developer as
 * unexpected results may occur. Instead, the user should reference
 * standards(7) for correct usage of the standards feature test macros.

But nevertheless, the thing in assert doesn't seem quite right.
With two highly dependant things (the macro that expands to a function call and the prototype for the function) using different conditions.

Here is a variant changing the assert headers to only use standard defines for both C and C++
wanders@d4fdb4f

[rmustacc]

As I understand it the c99 check is there for __func__ which was added in c99. Not the stringizing operator, which has been around before standardisation I think.

Ah, thanks. That makes sense then.

But nevertheless, the thing in assert doesn't seem quite right. With two highly dependant things (the macro that expands to a function call and the prototype for the function) using different conditions.

Here is a variant changing the assert headers to only use standard defines for both C and C++ wanders@d4fdb4f

Yeah, I agree that I screwed that up and they should be a single condition. If you're okay with it, I'll take this as basis to fix it upstream in illumos, leave you as the author, and make a small style change to make sure the ending comments for the if blocks match the guards. Let me know if that's reasonable to you and I appreciate you pointing out the things I've been missing here.

The broader discussion about what to do was more so for @jperkin and the question of taking the patch around the versions into illumos/gcc.

[wanders]

Yeah, I agree that I screwed that up and they should be a single condition. If you're okay with it, I'll take this as basis to fix it upstream in illumos, leave you as the author, and make a small style change to make sure the ending comments for the if blocks match the guards. Let me know if that's reasonable to you and I appreciate you pointing out the things I've been missing here.

Sounds good, feel free to take my patch!

If it is possible it would be appreciated if you could add me to CC in upstream ticket/review.

[rmustacc]

Yeah, I agree that I screwed that up and they should be a single condition. If you're okay with it, I'll take this as basis to fix it upstream in illumos, leave you as the author, and make a small style change to make sure the ending comments for the if blocks match the guards. Let me know if that's reasonable to you and I appreciate you pointing out the things I've been missing here.

Sounds good, feel free to take my patch!

If it is possible it would be appreciated if you could add me to CC in upstream ticket/review.

The upstream review will be on gerrit, so I can drop things here for you to track or shoot you an e-mail if you let me know how you'd like to be contacted. I'll try to have something up in the next few days, but there will be some variability due to some personal travel.

[wanders]

The upstream review will be on gerrit, so I can drop things here for you to track or shoot you an e-mail if you let me know how you'd like to be contacted.

The mail address used in author field in my commit should work fine.

[danmcd]

I am not travelling, but starting Tuesday night I'll be very high-latency&jitter due to the same reasons @rmustacc is travelling (US Thanksgiving holiday is Thursday, Friday's off too for many, and folks often travel for it), plus I'm testing out a biweekly SmartOS release a day later than normal because of that reason. That said I can help however I can as well.

[rmustacc]

Note, I had to make a minor functional change to the latest patch as the static assert guard in <iso/assert.h> still requires <sys/feature_tests.h> and I'd rather not rewrite it. I also added a comment explaining a bit about what's going on. I've got a build kicked off and I'll drop links to the upstream review later today.

I also was looking at the differences this might cause. It looks like C++98 and pkgsrc due to this will now end up getting the non-C99 assert though g++ and clang++ support this in C++98 and don't even generate a warning with -pedantic -std=c++98. I'll admit, it does make me tempted to not have the version part of the guard for folks, even though I realize it's not strictly by the standard. I have not done it, but I think it could make sense.

[rmustacc]

Upstream issue: https://www.illumos.org/issues/17765 and review: https://code.illumos.org/c/illumos-gate/+/4470.