Issue metadata
Sign in to add a comment
|
clang allows for loop initial declaration with -std=gnu89/c89 |
||||||||||||||||||||||||
Issue descriptionThe kernel CL:1064477 does a variable declaration in a for loop, a feature that was introduced in C99. Even though the Linux kernel is built with -std=gnu89 clang didn't raise an error. The CL made it past the CQ and later a test build with gcc failed. A simple test case is attached: gcc -std=gnu89 /tmp/test.c /tmp/test.c: In function ‘main’: /tmp/test.c:5:3: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode for (int i = 0; i < 10; i++) ^ /tmp/test.c:5:3: note: use option -std=c99, -std=gnu99, -std=c11 or -std=gnu11 to compile your code clang -std=gnu89 /tmp/test.c => success
,
Jun 5 2018
For a workaround, does the kernel compile successfully with `-Werror -Wc99-extensions`?
05-Jun 04:16PM [ 2.412s ] :: ~/chromeos » $clang /tmp/test.c -std=gnu89 -o /dev/null -c
05-Jun 04:16PM [ 47.101ms ] :: ~/chromeos » $clang /tmp/test.c -std=gnu89 -o /dev/null -c -Wc99-extensions
/tmp/test.c:5:8: warning: variable declaration in for loop is a C99-specific feature [-Wc99-extensions]
for (int i = 0; i < 10; i++)
^
1 warning generated.
(Looks like there's a handful of warnings that -Wc99-extensions enables; I'm unsure how that overlaps with what GCC allows in -std=gnu89... I'm also unsure how -std=gnu* is "standardized". If the answer is "whatever GCC allows", this probably won't be too hard to convince upstream to accept a fix for)
,
Jun 5 2018
'-Werror -Wc99-extensions' fails with a different bunch of warnings errors:
/mnt/host/source/src/third_party/kernel/v4.4/arch/arm64/include/asm/insn.h:58:35: error: commas at the end of enumerator
lists are a C99-specific feature [-Werror,-Wc99-extensions]
AARCH64_INSN_HINT_SEVL = 0x5 << 5,
^
/mnt/host/source/src/third_party/kernel/v4.4/arch/arm64/include/asm/insn.h:82:25: error: commas at the end of enumerator
lists are a C99-specific feature [-Werror,-Wc99-extensions]
AARCH64_INSN_REGTYPE_RA,
^
...
(Would be nice to move the kernel forward to at least a slightly more recent standard like C99, but probably this won't happen any time soon.)
,
Jun 6 2018
Sounds good. More broadly, it looks like clang accepts `for (int i = 0; ...)` when given -std=c89 (GCC doesn't), so while I'm told that `-std=gnu*`'s standard is "whatever GCC feels like", we'd probably not want -std=gnu89 to disable something that's available in -std=c89. The most backwards-compatible change is probably to add a new, more targeted -Wc89-for-loop-init that's implicitly enabled by -Wc99-extensions. (If the kernel doesn't build with -Werror to begin with, we can just pass -Werror=c89-for-loop-init to clang, as well.) If that lands in clang, would the upstream kernel be OK with passing that?
,
Jun 6 2018
(...Alternatively, it looks like we have a -Wgcc-compat warning already, which we might be able to work with. Can you check if the kernel builds with that, please?)
,
Jun 6 2018
-Wgcc-compat is already enabled and was already the source of our entertainment: http://crbug.com/709711 ;-) Knowing how reluctant the kernel community is about clang specific changes I think the preferred solution would be to disable 'for (int i = 0; ...)' for '-std=c89', since it effectively isn't part of the standard.
,
Jun 6 2018
From a purity point of view, I agree. My concern is that existing clang-based projects that use -std={c,gnu}89 may be broken by this. IME, clang in general tries to be accepting of code across standards (with warnings, as necessary) when the programmer's intent is clear.
I think we have a great case for putting a warning about this under -Wgcc-compat, though. The entertainment makes it look like -Werror is on, too, so whether we make it an actual error or a warning under -Wgcc-compat should make no difference to the kernel, no?
,
Jun 6 2018
distributing load. assigning this one to gbiv since he has done some initial analysis.
,
Jun 6 2018
#7: I agree that we should try to avoid breaking existing projects. Handling this case with -Wgcc-compat should indeed make no difference to the kernel and seems a suitable solution.
,
Jun 6 2018
SGTM; review: https://reviews.llvm.org/D47840
,
Jun 6 2018
Note that the kernel has some mistakes when being explicit about which C standard is used for most source files. Some Makefiles overwrite KBUILD_CFLAGS which makes the -std=* implicit based on compiler+compiler version. So it's entirely possible that the change could slip in if it was compiled with GCC 5.1 which changed the implicit default from gnu89 to gnu11. Unlikely and meaningless, but not impossible.
,
Jun 6 2018
Also, it was pointed out that if I can land my extern inline/gnu_inline changes, we may be able to upgrade the C standard used in the kernel (which would make gnu89 compat moot, but may be a lot of work). Just a thought.
,
Jun 28 2018
Should be fixed by clang's r335927. Will close this issue when our stable compiler pulls that revision in. (I'm assuming this isn't critical enough to cherry-pick, at least)
,
Sep 28
This should be fixed by the new compiler roll https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1246304 |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by manojgupta@chromium.org
, Jun 5 2018Components: Tools>ChromeOS-Toolchain