Use gnu17 for buildkernel

[minsoochoo0122_proton.me]

Summary

This is part 2 of adopting C17, therefore migrating to gnu17 from gnu99. This depends on D43237

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, _Alignof cannot be used in an expression according to standard C23
• Allowing programs to use C11 features without specifying CSTD= c17 or similar.

This revision does

• Change default C standard version for buildkernel to gnu17
• Replace _Alignof with __alignof__ to avoid error: '_Alignof' applied to an expression is a GNU extension

Test Plan

make kernel
kyua test -k /usr/tests/Kyuafile

[imp]

I like the direction, have a couple of concerns around testing and compiling with older compilers that some of our down streams might be stuck with...

sys/amd64/amd64/db_trace.c

208 ↗

(On Diff #132001)

I'd prefer this was unchanged and that we define _Alignof in terms of alignof for sufficiently new C standards in sys/cdefs.h. We already come close to doing it in cdefs.h (it defines it to __alignof() which is likely why the error happens. There's about 25 _Alignof in the kernel today and it will be confusing to people to know which to use...

sys/arm64/arm64/db_trace.c

95 ↗

(On Diff #132001)

ditto

sys/conf/kern.mk

370

so why gnu17 and not just plain c17?

i have similar patches that (A) set this to c11 and (b) Remove support for k&r, c89, c90, c94, and c95 pegging the minimum to compile the kernel at c99 (though I think jrtc27 and others would rather we go all the way to c11, which isn't a bad goal since some atomics are simplified).

Also, does the kernel boot or just compile. There's some subtle semantics changes between C standard revisions. Other than atomics, I think the kernel steers well clear of them, but you never know...

sys/contrib/zstd/lib/freebsd/assert.h

2

this looks unrelated.

2

Oh, I missed the last line... why did you need to copy this?

[minsoochoo0122_proton.me]

sys/conf/kern.mk

370

The goal of adopting gnu17 is not removing gnu extensions but being ready to adopt c23. c23 will be finalized in February 2024 (c23 schedule) and once it is publicly released, gnu23 can be adopted and then the kernel can be migrated to c23.

The reason for this is that there are some cases of using __attributes__(unused) which can be replaced with [[ maybe_unused ]] (explanation on cppreference.com) in c23.

sys/contrib/zstd/lib/freebsd/assert.h

2

src/sys/contrib/zstd/lib/common/xxhash.h:1354

When __STDC_VERSION__ >= 201112L, we need assert.h. However, header files like stdio.h is not included from src/include but we mock userspace header files by creating an equivalent to src/sys/contrib/zstd/lib/freebsd. This has been done since zstd was first imported to kernel in d3692a4dee4d6ff0926365bcf95bbedb039ea70d.

assert() is a macro function so I though copying and paste it to src/sys/contrib/zstd/lib/freebsd would be better than integrating assert() in zstd_kfreebsd.h

[minsoochoo0122_proton.me]

sys/conf/kern.mk

370

The system compiles and boots when buildworld and buildkernel are bumped to gnu17.

I'm running kyua tests right now which will be done in a few hours.

[arichardson]

Overall I'd be very happy to see the standard bumped to c17 but I'm not sure what the current minimum compiler versions are. But then again maybe it's time to say you need a c17 compiler for current...

sys/amd64/amd64/db_trace.c

208 ↗

(On Diff #132001)

I'm surprised we're seeing '_Alignof' applied to an expression is a GNU extension with the gnu17 flag? Is this file compiled with other flags or does that warning not get silenced by using the gnu versions of the -std flag.

sys/conf/kern.mk

370

We use a lot of GNU extensions so using c17 would most likely require adding a bunch of -Wno-foo flags

[minsoochoo0122_proton.me]

sys/amd64/amd64/db_trace.c

208 ↗

(On Diff #132001)

Clang diagnostic flags
This warning is turned on by default.

[minsoochoo0122_proton.me]

sys/amd64/amd64/db_trace.c

208 ↗

(On Diff #132001)

I'd prefer this was unchanged and that we define _Alignof in terms of alignof for sufficiently new C standards in sys/cdefs.h. We already come close to doing it in cdefs.h (it defines it to __alignof() which is likely why the error happens. There's about 25 _Alignof in the kernel today and it will be confusing to people to know which to use...

_Alignof is an operator and alignof is already a C11 macro in <stdalign.h>, which will be an operator in C23. Should I replace them with __alignof()?

[jhb]

sys/amd64/amd64/db_trace.c

208 ↗

(On Diff #132001)

So <sys/cdefs.h> is already correct I think in that it doesn't define _Alignof at all but lets it use the native language version for C11.

The error here is that _Alignof/alignof can only be used on types. In C23 the way you would spell this is alignof(typeof(*tf)). Instead, I think the way to fix these is to use _Alignof(struct trapframe) which will be valid in C11 and not require typeof. Fixing the various db_trace.c to use struct trapframe instead of *tf just needs to be its own commit to fix my earlier change to be valid C11.

sys/conf/kern.mk

370

We are years away from requiring gnu23 to build the kernel I think. We support older toolchains (e.g. right now main still builds with clang 12). We can't feasibly get by with requiring the latest version of toolchains. We don't have a formal policy on this currently, and we could talk about that on, say, arch@. c17 is probably the newest thing we could require, and c11 is probably an easier pitch currently.

[freebsd_igalic.co]

sys/conf/kern.mk

370

c17 is probably the newest thing we could require, and c11 is probably an easier pitch currently.

my pitch for c17 is: it's c11, actually (with the most glaring bugs fixed)

[imp]

sys/conf/kern.mk

370

I think that we can easily sell c11. If we enumerate the things that are actually fixed, we can likely sell c17. Especially if there's a number of tangible, easy to articulate benefits. It gets dicier if the reasons are 'it's the newest' or 'trust me it's better' and the 'most of the bugs fixed' level of detail is more on the dicier end of the scale. I don't doubt it, but it would be helpful to enumerate what those things are and how it would help us, the FreeBSD project, have a better system.

[jhb]

sys/conf/kern.mk

370

Yes, GDB just recently went through this where we moved from requiring C++11 to C++17 and in that case there some specific things that GDB had already backported (std::optional<> and array_view come to mind) that helped justify sliding forward. In GDB's case the policy they use is that supported LTS OS versions have to have a compiler that supports the version of the language (or some such).

For FreeBSD I think we'd like to continue supporting cross-builds using external toolchains on supported versions of Linux and macOS, so would have some similar constraints in how quickly we can step our minimum toolchain requirements up.

[arichardson]

I had a look on godbolt to see which compiler versions support c17: https://godbolt.org/z/EGd4TqP3j

It seems that GCC 8.1 and Clang 6.0 are the oldest compilers that accept the flag value. Both of those are sufficiently old that I believe requiring C17 instead of C11 should be safe?

sys/conf/kern.mk

370

It appears the list of changes between C11 and C17 is https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2244.htm I haven't looked through all of them but it seems like there are quite a few clarifications/fixes to the atomics specification.

sys/contrib/zstd/lib/freebsd/assert.h

2

If this is really needed, could this file just`#include "../../../../include/assert.h" instead of copying the file?

I guess the alternative would be to modify the xxhash.h file to use _Static_assert instead of including <assert.h>?

[arichardson]

Looking at the Clang change that added -std=c17 (https://github.com/llvm/llvm-project/commit/5b6c0f75e01571851b767dc63a3229c962f464f1), the only difference is the value of __STDC_VERSION__, so unless code uses this in preprocessor macros it's identical to -std=c11

[minsoochoo0122_proton.me]

sys/conf/kern.mk

370

I think that we can easily sell c11. If we enumerate the things that are actually fixed, we can likely sell c17. Especially if there's a number of tangible, easy to articulate benefits. It gets dicier if the reasons are 'it's the newest' or 'trust me it's better' and the 'most of the bugs fixed' level of detail is more on the dicier end of the scale. I don't doubt it, but it would be helpful to enumerate what those things are and how it would help us, the FreeBSD project, have a better system.

I agree with this. The only reason for adopting c23 is to stopping depending on GNU C extensions. Introduction of alignof, alignas, and typeof in c23 can fix most of the problems.

sys/contrib/zstd/lib/freebsd/assert.h

2

IMO we should separate kernel-specific codes from user space as sys/contrib is separate from contrib.
I'm trying not to modify files fetched from zstd release. In this way, it is much easier to import next releases of zstd.

[minsoochoo0122_proton.me]

Differences between c* and gnu* flags are listed here.

[minsoochoo0122_proton.me]

$ cd/usr/src/sys
$ grep -r _Alignof
./vm/swap_pager.c:	    NULL, NULL, _Alignof(struct swblk) - 1, 0);
./vm/uma.h:#define	UMA_ALIGNOF(type) (_Alignof(type) - 1)	/* Alignment fit for 'type' */
./riscv/riscv/db_trace.c:			if (!__is_aligned(tf, _Alignof(struct trapframe)) ||
./ufs/ffs/ffs_vnops.c:	(((uintptr_t)(ptr) & (_Alignof(s) - 1)) == 0)
./netinet/libalias/alias_proxy.c:	_Alignas(_Alignof(u_short)) u_char option[OPTION_LEN_BYTES];
./netinet/ip_fw.h:	_Alignas(_Alignof(u_int32_t)) u_int8_t 	opcode;
./arm64/arm64/vfp.c:	    _Alignof(struct vfpstate) - 1, 0);
./arm64/arm64/db_trace.c:			if (!__is_aligned(tf, _Alignof(struct trapframe)) ||
./sys/cdefs.h:#define	_Alignof(x)		alignof(x)
./sys/cdefs.h:#define	_Alignof(x)		__alignof(x)
./sys/_types.h:	long long __max_align1 __aligned(_Alignof(long long));
./sys/_types.h:	long double __max_align2 __aligned(_Alignof(long double));
./amd64/amd64/db_trace.c:	if (!__is_aligned(tf_addr, _Alignof(struct trapframe)) || !INKERNEL(tf_addr)) {
./amd64/amd64/db_trace.c:		if (!__is_aligned(frame, _Alignof(struct amd64_frame)) ||
./amd64/include/vmm.h:_Static_assert(_Alignof(struct vie_op) == 2, "ABI");
./netlink/netlink_message_parser.h:} __aligned(_Alignof(__max_align_t));
./netlink/netlink_message_parser.h:	len = roundup2(len, _Alignof(__max_align_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(uint8_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(uint8_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(uint8_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(uint16_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(uint32_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(uint64_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(int8_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(int16_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(int32_t));
./dev/bhnd/nvram/bhnd_nvram_subr.c:		return (_Alignof(int64_t));

I modified db_trace_c that _Alignof takes struct trapframe instead of *tf.
It seems that db_trace.c is the only file that uses expression in _Alignof.

[minsoochoo0122_proton.me]

Fixed _Alignof()

[minsoochoo0122_proton.me]

I guess this is now ready to go.

[imp]

In D43239#986558, @minsoochoo0122_proton.me wrote:

I guess this is now ready to go.

All except for community but in, sure.... hence my coaching on how to sell it.

[imp]

And I'm not sold on the copying of assert.h

[minsoochoo0122_proton.me]

Simplifying assert.h

[minsoochoo0122_proton.me]

Since zstd only uses static_assert, I only took static_assert macro from include/assert.h

This is the minimum version of assert.h that can be used in this context.

[jhb]

This probably should be a series of commits, not one commit. In particular, the _Alignof() fix needs to be its own fix at the very least. I can help with that.

Re: gnu17 vs c17, this is really a question for GCC not clang as clang is less strict and enables some GNU extensions in c99 that GCC only enables in gnu99. See the commit log that switched to gnu99 only recently: ec41a96daaa6e401bc0d4ba71d9cf37a1d79fc86. Note that this also brings up another point which is that this needs testing with GCC, not just clang.

[jhb]

sys/contrib/zstd/lib/freebsd/assert.h

2

Actually, what I'd like to do for the kernel is just go ahead and define static_assert() in <sys/systm.h> (really <sys/kassert.h>) instead of requiring the use of _Static_assert in the kernel. The intention of C11 from my reading is that code should really be using static_assert and that _Static_assert is just a shim so that <assert.h> can define the wrapper macro. Honestly I'd prefer it if for the kernel if we eventually too a similar route with defining things like alignof in the kernel always and not having to use the more obfuscated _Alignof type keywords. In the case of static_assert fixing that would probably mean we wouldn't need this hack at all for zstd to compile in the kernel.

[minsoochoo0122_proton.me]

Divide _Alignof part to D43409

[minsoochoo0122_proton.me]

Remove unnecessary include

[minsoochoo0122_proton.me]

sys/contrib/zstd/lib/freebsd/assert.h

2

_Static_assert() is a compiler extension in clang, so I guess there is no need to redefine it.

[minsoochoo0122_proton.me]

sys/contrib/zstd/lib/freebsd/assert.h

2

sys/cdefs.h

#if !__has_extension(c_static_assert)
#if (defined(__cplusplus) && __cplusplus >= 201103L) || \
    __has_extension(cxx_static_assert)
#define	_Static_assert(x, y)	static_assert(x, y)
#elif __GNUC_PREREQ__(4,6) && !defined(__cplusplus)
/* Nothing, gcc 4.6 and higher has _Static_assert built-in */
#elif defined(__COUNTER__)
...

[jhb]

sys/contrib/zstd/lib/freebsd/assert.h

2

Yes, but <sys/cdefs.h> is a generic header for more than just the kernel. We define kernel-specific things in <sys/systm.h>. And I think we should just #define static_assert to _Static_assert in <sys/systm.h> under #ifdef KERNEL.

[minsoochoo0122_proton.me]

Move the definition of static_assert in <sys/systm.h>
Previous definition for linux compatibility is removed due to type definition conflict

[minsoochoo0122_proton.me]

Are there any more changes required?

[minsoochoo0122_proton.me]

Fixed macro

[minsoochoo0122_proton.me]

Are there any blocking issues for this pr? If not, can we commit the patch to the repo?

[jhb]

So the static_assert() change needs to go in first I think which is all but the CSTD change as a commit.

[minsoochoo0122_proton.me]

• Add static_assert for kernel
• Use gnu17 for buildkernel

[minsoochoo0122_proton.me]

In D43239#1095174, @jhb wrote:

So the static_assert() change needs to go in first I think which is all but the CSTD change as a commit.

Completed splitting into two commits.