Page MenuHomeFreeBSD

Use gnu17 for buildworld
AcceptedPublic

Authored by minsoochoo0122_proton.me on Dec 29 2023, 9:56 PM.
Referenced Files
F84154509: D43237.diff
Mon, May 20, 2:40 AM
Unknown Object (File)
Fri, May 10, 3:50 AM
Unknown Object (File)
Thu, May 9, 6:20 PM
Unknown Object (File)
Thu, May 9, 2:23 PM
Unknown Object (File)
Wed, May 8, 8:48 AM
Unknown Object (File)
Fri, Apr 26, 9:29 PM
Unknown Object (File)
Mon, Apr 22, 10:54 PM
Unknown Object (File)
Apr 4 2024, 5:15 PM

Details

Summary

This is part 1 of adopting C17, therefore migrating to gnu17 from gnu99.

Why?
There are many reasons, but some prominent reasons are

  • Facilitating the adoption of C23/C2x. This is likely to be finalized in 2024, and clang has already implemented most of C2x features. Migrating from C17 to C2x will be easier than migrating from C99 to C2x.
  • C11 has adopted GCC specific features as a part of standard, such as _Alignas. However, _Alignas currently conflicts with gnu17 when used without -Wno-gnu-statement-expression
  • Allowing programs to use C11 features without specifying CSTD= c17 or similar.

This revision DOES

  • Change default C standard version for buildworld to gnu17
  • Remove CSTD= c11 in some Makefiles since gnu17 will be automatically applied

This revision DOES NOT

  • Change C standard version in contributed code in contrib/

From now own, the mimum required compiler version for is Clang 7.0.0 and GCC 8.1.0

Test Plan

make buildworld && make installworld

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 55203
Build 52092: arc lint + arc unit

Event Timeline

What is the minimum Clang version (and GCC version) needed for this?

What is the minimum Clang version (and GCC version) needed for this?

Clang C support status

Clang 3.3 and later have implemented all of the C11 features, and -std=c17 was added in Clang 6. So I guess that -std=gnu17 is also available since Clang 6.

However, C11 features have been already used in the base system, and this revision does not add any code that requires C11 features.

Yea, I had vague plans on bumping this to c11 universally as well...

I'd rather we deal with the _Alignof issue as I suggested in the other review.

And we likely need some kind of regression test to ensure that we can still build c89, c99, c11 programs because we have a lot of ports that are at least compiling with c99 and some that do c89 for 'maximum portability' so our headers have to remain compatible over the long haul.

Again, why gnu17 instead of c17 is a question in my mind. Also, does a resulting system work?

also, I don't see how contrib is omitted.

In D43237#985660, @imp wrote:

Yea, I had vague plans on bumping this to c11 universally as well...

I'd rather we deal with the _Alignof issue as I suggested in the other review.

And we likely need some kind of regression test to ensure that we can still build c89, c99, c11 programs because we have a lot of ports that are at least compiling with c99 and some that do c89 for 'maximum portability' so our headers have to remain compatible over the long haul.

Again, why gnu17 instead of c17 is a question in my mind. Also, does a resulting system work?

The new system compiles and boots. I'm currently testing with kyua test -k /usr/tests/Kyuafile

In D43237#985661, @imp wrote:

also, I don't see how contrib is omitted.

Sorry my summary was somewhat inaccurate. I meant that unless codes in contrib/ specified a specific CSTD value, contrib/ will also follow gnu17. For base systems without contrib/, gnu17 will be applied.

Currently there is a bug where a regression test for tgmath (which uses c89) fails. This test has not been updated since 2004, so I don't know if this was already broken.

In D43237#985660, @imp wrote:

Again, why gnu17 instead of c17 is a question in my mind. Also, does a resulting system work?

There is no replacement for GCC extensions like typeof in c17, but it will be added in c23 as standard.

I'd like to see a patch doing what i described for supported version. Needn't be in this review but should be in the series.

share/mk/bsd.sys.mk
23

If we do this, we should deorbit support for < c11 here. Just delete all the old versions here and in similar code in the kernel build.

This revision is now accepted and ready to land.Dec 31 2023, 3:52 AM

I'd also fix the commit message to include the details in my questions. Might need to finesse it a bit and likely a conversation on arch@. But the more details that are right before will help that go well. I'm keen to keep gnu11 working as a fallback at least since these sorts of transitions work better if there is a quick fallback plan to do a/b testing in case of unforeseen problems after the commit. Given these changes i see that as working... it makes a better plan for people to get behind and to mollify the knee jerk conservatism that might pop up.

In D43237#985747, @imp wrote:

I'd like to see a patch doing what i described for supported version. Needn't be in this review but should be in the series.

D43254 will remove supports for < C11 codes.
Please note that D43236 must be landed first otherwise build will fail on D43254.

arichardson added inline comments.
share/mk/bsd.sys.mk
12–13

This comment needs to be updated (although IMO it doesn't add much value).

minsoochoo0122_proton.me edited the summary of this revision. (Show Details)

Remove unnecessary comment

This revision now requires review to proceed.Jan 2 2024, 10:26 PM
minsoochoo0122_proton.me added inline comments.
share/mk/bsd.sys.mk
12–13

I just removed the comment because IMO it is clear that we are using gnu17.

23

Discussing in D43254

This LGTM but the commit message should also include the minimum Clang+GCC versions

This revision is now accepted and ready to land.Jan 30 2024, 12:01 AM