-Wvla in __compiletime_assert

[kees]

Without support for compile-time warnings, Clang is forced to fall back to a build-failure using __compiletime_error_fallback in include/linux/compiler.h:

    #  define __compiletime_error_fallback(condition) \
            do { ((void)sizeof(char[1 - 2 * condition])); } while (0)

However, this is a VLA, so future use of -Wvla will make builds extraordinarily noisy. This could likely be worked around in some Clang-specific way, but having true compile time warnings would be best, as that means Clang builds gain sensible warnings for things like BUILD_BUG_ON_MSG().

[nickdesaulniers]

Doesn't Clang support #error 'msg'?

[kees]

Yes, but that can't be assigned to functions the way BUILD_BUG_ON() and CONFIG_FORTIFY_SOURCE uses those constructs.

[nickdesaulniers]

sorry, read this too quickly, missed that this was specifically about function attributes.

[gburgessiv]

Might diagnose_if do what you want? It's what's used to implement FORTIFY for clang outside of the kernel, and can do both errors and warnings.

[kees]

Oooh! Yes, that works great! Now I just have to figure out why this is normally silent...

./include/linux/debug_locks.h:17:9: error: Bad argument size for xchg
        return xchg(&debug_locks, 0);
               ^
./arch/x86/include/asm/cmpxchg.h:78:22: note: expanded from macro 'xchg'
#define xchg(ptr, v)    __xchg_op((ptr), (v), xchg, "")
                        ^
./arch/x86/include/asm/cmpxchg.h:67:4: note: expanded from macro '__xchg_op'
                        __ ## op ## _wrong_size();                      \
                        ^
<scratch space>:29:1: note: expanded from here
__xchg_wrong_size
^
./arch/x86/include/asm/cmpxchg.h:14:2: note: from 'diagnose_if' attribute on '__xchg_wrong_size':
        __compiletime_error("Bad argument size for xchg");
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/compiler-clang.h:40:17: note: expanded from macro '__compiletime_error' 
        __attribute__((diagnose_if(1, message, "error")))
                       ^           ~

[kees]

Looks like Clang isn't eliminating the case statement in __xchg_op()...

[gburgessiv]

Right. clang's diagnose_if will complain at each callsite to the function where the condition is trivially constant and evaluates to true, regardless of its reachability after optimizations. So, in cases like cmpxchg, you'll need to find a way to funnel the condition into the function's condition, sadly. :/

e.g.

void __cmpxchg_wrong_size_if(int a) __attribute__((diagnose_if(a, ...)));

__cmpxchg_wrong_size_if(sizeof(x) != sizeof(y)); // or whatever the condition is.

[gburgessiv]

(In general, clang's architected in such a way that by the time optimizations start to happen, emitting diagnostics is hard. It's not impossible to do so, but it's often not pretty, either)

[kees]

This code:

#include <stdio.h>

extern void whoops(void) __attribute__((diagnose_if(1, "text goes here", "warning")));

int main(void)
{
        int r = 5;

        switch (sizeof(r)) {
        case 1:
                printf("one\n");
                break;
        case 2:
                printf("two\n");
                break;
        case 4:
                printf("four\n");
                break;
        case 8:
                printf("eight\n");
                break;
        default:
                whoops();
        }

        return 0;
}

Still warns:

$ clang-6.0 -O2 -Wall -o warn warn.c
warn.c:23:3: warning: text goes here [-Wuser-defined-warnings]
                whoops();
                ^
warn.c:3:41: note: from 'diagnose_if' attribute on 'whoops':
extern void whoops(void) __attribute__((diagnose_if(1, "text goes here"...
                                        ^           ~
1 warning generated.

Even though the line isn't actually used in output:

00000000004004f0 <main>:
  4004f0:       50                      push   %rax
  4004f1:       bf 84 05 40 00          mov    $0x400584,%edi
  4004f6:       e8 e5 fe ff ff          callq  4003e0 <puts@plt>
  4004fb:       31 c0                   xor    %eax,%eax
  4004fd:       59                      pop    %rcx
  4004fe:       c3                      retq   
  4004ff:       90                      nop

[kees]

The kernel's __compiletime_assert() mechanism assumes dead-code elimination has happened. Hmmm.

[gburgessiv]

#define __compiletime_assert(condition, msg, prefix, suffix)		\
	do {								\
		bool __cond = !(condition);				\
		extern void prefix ## suffix(int __cond) __compiletime_error_cond(__cond, msg); \
		if (__cond)						\
			prefix ## suffix(__cond);				\
		__compiletime_error_fallback(__cond);			\
	} while (0)

?

(Assuming __compiletime_error_cond goes to something that uses diagnose_if(__cond, ... for clang, and is just #define __compiletime_error_cond(_, msg) __compiletime_error(msg) for GCC)

[kees]

Hunh. There is only a single use of __compiletime_warning() in the kernel, so that would be easy to change.

__compiletime_error() is relatively rare too, and fixing BUILD_BUG_ON() should be easy given it has the condition in it already.

But... this doesn't seem to work still:

#include <stdio.h>

void whoops(size_t size) __attribute__((diagnose_if(size != 0, "text goes here", "warning"))) {
        printf("bad\n");
}

int main(void)
{
        int r = 5;
        size_t size = sizeof(r);

        switch (size) {
        case 1:
                printf("one\n");
                break;
        case 2:
                printf("two\n");
                break;
/*
        case 4:
                printf("four\n");
                break;
*/
        case 8:
                printf("eight\n");
                break;
        default:
                whoops(size);
        }

        return 0;
}

$ clang-6.0 -O2 -Wall -o warn warn.c
$ ./warn 
bad

There's still no warning generated. :(

[gburgessiv]

It should work if you make size a const size_t. diagnose_if checks happen early in compilation, so have to use the constexpr evaluator in clang to figure out what the things evaluate to. Non-const variables (even if they're basically constant, like in your example) generally make that evaluator give up very quickly.

[kees]

Still doesn't work, the dead-code elimination still happens too late. I'll have to redesign the kernel's __compiletime_error_fallback() to avoid a VLA for future -Wvla.

[nickdesaulniers]

@kees and I reviewed code that removes this fallback: https://lore.kernel.org/r/1535220989-27645-1-git-send-email-yamada.masahiro@socionext.com/

[dileks]

Just FYI:

Makefile: Globally enable VLA warning
compiler.h: give up __compiletime_assert_fallback()

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/vla

[nickdesaulniers]

Linus just merged commit 81b4568 ("compiler.h: give up __compiletime_assert_fallback()") into 4.20. Thanks @kees for picking up that patch and sending it along in a pull request!