-Wdeclaration-after-statement in arm_neon.h

[nathanchance]

The Linux kernel is looking to upgrade from -std=gnu89 to -std=gnu11. The kernel builds with -Wdeclaration-after-statement, which exposes an issue in arm_neon.h with arm64 big endian: https://godbolt.org/z/313sW6zrW

$ cat repro.c
#include <arm_neon.h>

static inline uint8x16_t SHLBYTE(uint8x16_t v)
{
    return vshlq_n_u8(v, 1);
}

$ clang --target=aarch64-linux-gnu -mbig-endian -std=gnu11 -Wdeclaration-after-statement -fsyntax-only -ffreestanding repro.c
repro.c:5:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
        return vshlq_n_u8(v, 1);
               ^
.../lib/clang/15.0.0/include/arm_neon.h:24823:14: note: expanded from macro 'vshlq_n_u8'
  uint8x16_t __ret; \
             ^
1 warning generated.

$ clang --target=aarch64-linux-gnu -mbig-endian -std=gnu89 -Wdeclaration-after-statement -fsyntax-only repro.c

As far as I understand based on reading clang::ProcessWarningOptions() and DiagnosticIDs::getDiagnosticSeverity(), there should not be any warnings from system headers if -Wsystem-headers is not passed, which the kernel does not do.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[DavidSpickett]

I don't think this issue is specific to this warning.

This "warning" changed recently:
https://reviews.llvm.org/D117232
https://reviews.llvm.org/rG118f966b46cfb60897b56a9878e1c68fd0e2afa4

Now when you are C99 or later it is a Warning vs. an Extension. (which means a warning or note about an extension from a later standard?)

The issue applies to other warnings too, at least coming from a macro in a system header. I haven't checked without the macro around them.

If I modify the little endian macro to add an unused variable and enable that warning I get:

$ ./bin/clang /tmp/test.c -c -o /tmp/test.o -Wunused -Wno-system-headers
/tmp/test.c:5:12: warning: unused variable 'a' [-Wunused-variable]
    return vshlq_n_u8(v, 1);
           ^
<...>build-llvm-aarch64/lib/clang/15.0.0/include/arm_neon.h:24814:7: note: expanded from macro 'vshlq_n_u8'
  int a; \
      ^
/tmp/test.c:3:26: warning: unused function 'SHLBYTE' [-Wunused-function]
static inline uint8x16_t SHLBYTE(uint8x16_t v)
                         ^
2 warnings generated.

I did a bit of printf hacking and it seems that clang is well aware that arm_neon.h is a system header and that the code comes from a macro within it (by using the "Spelling" location of the code the macro generates). However when it asks if the specific warning should be suppressed in system headers, the answer is no.

I thought perhaps it was a missing field in the way this new warning was added but I see very few explicitly put SuppressInSystemHeader/SuppressInSystemMacro. You can "fix" this by doing that but it seems like the wrong path.

So I think there's something going on where warnings in general aren't suppressed for system headers and/or macros within them.

[DavidSpickett]

Note to self (mostly), https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-system-header-prefix is an easier way to test this than hacking that header.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html for gcc's description of -Wsystem-headers. Couldn't find a clang page for it but I assume mostly the same.

[DavidSpickett]

I think the macro is the differentiation here.

Test adapted from clang/test/Frontend/system-header-prefix.c.

We have an unused var in a macro and in a function in this header:

$ cat foo/foo.h 
#define FOO int a; int b

inline void foo(void) {
  int z;
}

Then we include it and use both of them:

$ cat main.c
#include "foo.h"

int main() {
  FOO;
  foo();
  return 0;
}

Compile with foo/ as a system header location, warn for unused, no system header warnings. We get a warning about the macro despite it originating in the system header foo.h.

$ ./bin/clang main.c -c -o main.o -Wunused -I./foo --system-header-prefix foo -Wno-system-headers
main.c:4:3: warning: unused variable 'a' [-Wunused-variable]
  FOO;
  ^
./foo/foo.h:1:17: note: expanded from macro 'FOO'
#define FOO int a; int b
                ^
main.c:4:3: warning: unused variable 'b' [-Wunused-variable]
./foo/foo.h:1:24: note: expanded from macro 'FOO'
#define FOO int a; int b
                       ^
2 warnings generated.

If we enable system header warnings we get warnings from the function and the macro.

$ ./bin/clang main.c -c -o main.o -Wunused -I./foo --system-header-prefix foo -Wsystem-headers
In file included from main.c:1:
./foo/foo.h:4:7: warning: unused variable 'z' [-Wunused-variable]
  int z;
      ^
main.c:4:3: warning: unused variable 'a' [-Wunused-variable]
  FOO;
  ^
./foo/foo.h:1:17: note: expanded from macro 'FOO'
#define FOO int a; int b
                ^
main.c:4:3: warning: unused variable 'b' [-Wunused-variable]
./foo/foo.h:1:24: note: expanded from macro 'FOO'
#define FOO int a; int b
                       ^
3 warnings generated.

If the warning from the system header function is ignored, the macro warning should also be ignored.

[nathanchance]

c4db521 mentions:

The current behavior is kept in this patch
(i.e. warnings from system macros are enabled by default).
The decision as to whether this should be an opt-in or opt-out
feature can be made in a separate patch.

The review comments make it seem like disabling warnings in macros from system headers is a little tricky so maybe it is better to just special case this?

[DavidSpickett]

Good find! Yes, this will fix this specific issue:

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 15487260eb73..7a8204dd5648 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9876,7 +9876,7 @@ def ext_mixed_decls_code : Extension<
   InGroup<DeclarationAfterStatement>;
 def warn_mixed_decls_code : Warning<
   "mixing declarations and code is incompatible with standards before C99">,
-  InGroup<DeclarationAfterStatement>, DefaultIgnore;
+  InGroup<DeclarationAfterStatement>, DefaultIgnore, SuppressInSystemMacro;

 def err_non_local_variable_decl_in_for : Error<
   "declaration of non-local variable in 'for' loop">;

However if https://reviews.llvm.org/D116833#3236209 is right and there is any chance of missing a legit warning elsewhere, we should change arm_neon.h to use the older style.

[nathanchance]

Right, I think that moving arm_neon.h to the older style is the lower risk change that would sufficiently resolve this from the kernel's perspective, as after https://git.kernel.org/linus/04e85bbf71c9072dcf0ad9a7150495d72461105c, it does not use system headers, aside from the few intrinsic headers.

[nickdesaulniers]

Intrinsic::emitBody in clang/utils/TableGen/NeonEmitter.cpp looks like that's what builds up the return variable declaration statement.

[nathanchance]

Intrinsic::emitArgumentReversal() emits the declaration and Intrinsic::emitReverseVariable() emits the assignment.

I guess just combining those two functions so that the variable is both declared and assigned at the same time would be enough to silence the warning?

[nickdesaulniers]

1. https://reviews.llvm.org/D122188
2. https://reviews.llvm.org/D122189