Feature #14252
closed
Adjust boolean_t under __EXTENSIONS__
100%
Description
Historically the boolean_t type has tried to hide itself and change the names of its types while under an XPG specification. This is distinct from other visibility things as it doesn't change based on the use of __EXTENSIONS__. Normally this would be fine; however, clang in its current default form decides to start by specifying both _XOPEN_SOURCE=600 and __EXTENSIONS__. Whether that should be the default on our platform is a reasonable and different question. This causes a lot of havoc. Software that used to use the boolean_t in a normal env, is now stuck in the XPG only variant which changes the names. That doesn't help anyone.
In this case, I think we should do two things to help this situation out:
1. Make this check for the strict boolean_t not apply if __EXTENSIONS__ has been specified. Note, this does not change us to use the _STRICT_SYBMOLS function because that would change things.
2. We should define both versions of the names in the default environment. That means defining B_TRUE, _B_TRUE, B_FALSE, and _B_FALSE. This should be fairly safe since both versions have the same value. We believe that this shouldn't result in any -Wswich warnings or related.
To help prove this out, I've done a few different things with a bunch of help:
1. I built and wsdiff'd illumos-gate with this change. While a lot of changes show up in wsdiff because the new names cause the internal GCC count to increase for local symbols, it critically does not change any program text.
2. Toomas has verified that a bunch of this unblocks some of his current clang pieces.
3. We haven't seen any regressions in OmniOS bulk builds and a pkgsrc bulk build is still ongoing.
Overall, I believe this can lead us to this being a fairly safe change. Software can still opt to use bool instead or not in c99 mode, but this at least gets us going and solves some things. Finally, regardless of how clang ends up setting pre-processor flags by default, I believe this is still a reasonable change to make and will help a bunch of existing code such as the bits in cmd-inet which define __EXTENSIONS__ but have to use the _ versions.
Updated by Robert Mustacchi almost 2 years ago
This has been tested in a few ways:
- Toomas verified that the current clang default result in something that now doesn't require us to change everything in the system.
- Jonathan Perkin went through and did a large set of pkgsrc bulk builds. There were two failures. In Jonathan's words: The first is just incredibly old software that tries to define its own boolean_t implementation. The second tries to define its own B_FALSE/B_TRUE which we can patch around, and notably isn't a new failure, it fails in the same way without the patch.
- Similarly, Dominik was able to run OmniOS packages through a similar build and there were no additional builds or fall out.
- I did a wsdiff of everything. While there was a lot of data and output from this. Understandably, we found CTF differences in a lot of things and similarly in the string table. However, aside from a weird difference in libshell program text (which was just relocation offsets), nothing else changed. The strtab changes ultimately all made sense because the change caused gcc's mechanism for assigning local symbols internal numbers to vary. The final set of diffs were:
3 NOTE: ASCII difference detected. 503 NOTE: ELF .SUNW_ctf difference detected. 2 NOTE: ELF .data difference detected. 1 NOTE: ELF .dynamic difference detected. 3 NOTE: ELF .rodata difference detected. 1000 NOTE: ELF .strtab difference detected. 1 NOTE: ELF .text difference detected.
The .data and .rodata differences were because of a change to the version string. The .dynamic was due to a change in grub's checksum code. The ASCII changes were mostly the version string and the actual change to sys/types.h itself!
Updated by Electric Monk almost 2 years ago
- Status changed from New to Closed
- % Done changed from 0 to 100
git commit eb96f8f799ef5344a5736dd0f1721bc2ee59f980
commit eb96f8f799ef5344a5736dd0f1721bc2ee59f980
Author: Robert Mustacchi <rm@fingolfin.org>
Date: 2021-12-03T01:24:50.000Z
14252 Adjust boolean_t under __EXTENSIONS__
Reviewed by: Peter Tribble <peter.tribble@gmail.com>
Reviewed by: Toomas Soome <tsoome@me.com>
Approved by: Dan McDonald <danmcd@joyent.com>