ClangBuiltLinux/linux

Improve BUILD_BUG_ON error message with clang

pcc opened this issue · 10 comments

pcc commented

With clang BUILD_BUG_ON failures only appear at link time due to a lack of support for __attribute__((error)). The error messages look like this:

ld.lld: error: undefined symbol: __compiletime_assert_296
>>> referenced by signal_compat.c
>>>               kernel/signal_compat.o:(sigaction_compat_abi) in archive arch/x86/built-in.a

and it can be hard to trace them back to the caller.

I had the idea of putting the error message in the symbol name so that it survives until link time. Here's a quick proof of concept:

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 4e035aca6f7e..f553ad004e5b 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -109,10 +109,9 @@
                (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
        })
 
-extern void __compiletime_error("value doesn't fit into mask")
-__field_overflow(void);
-extern void __compiletime_error("bad bitfield mask")
-__bad_mask(void);
+extern void __field_overflow(void)
+       __compiletime_error("value doesn't fit into mask");
+extern void __bad_mask(void) __compiletime_error("bad bitfield mask");
 static __always_inline u64 field_multiplier(u64 field)
 {
        if ((field | (field - 1)) & ((field | (field - 1)) + 1))
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index cee0c728d39a..f4ad81f6f75e 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -61,3 +61,5 @@
 #if __has_feature(shadow_call_stack)
 # define __noscs       __attribute__((__no_sanitize__("shadow-call-stack")))
 #endif
+
+#define __compiletime_error(msg) __asm__(msg)
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index e93e249a4e9b..e6476b1be8cc 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -124,10 +124,10 @@ static inline void check_object_size(const void *ptr, unsigned long n,
 { }
 #endif /* CONFIG_HARDENED_USERCOPY */
 
-extern void __compiletime_error("copy source size is too small")
-__bad_copy_from(void);
-extern void __compiletime_error("copy destination size is too small")
-__bad_copy_to(void);
+extern void __bad_copy_from(void)
+       __compiletime_error("copy source size is too small");
+extern void __bad_copy_to(void)
+       __compiletime_error("copy destination size is too small");
 
 static inline void copy_overflow(int size, unsigned long count)
 {

With that and an artificial BUILD_BUG_ON failure injected into the kernel:

diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
index 862e69f6054f..5dfba690cf6b 100644
--- a/arch/x86/kernel/signal_compat.c
+++ b/arch/x86/kernel/signal_compat.c
@@ -164,7 +164,7 @@ static inline void signal_compat_build_tests(void)
        BUILD_BUG_ON(offsetof(siginfo_t, si_arch)      != 0x1C);
        BUILD_BUG_ON(offsetof(compat_siginfo_t, si_call_addr) != 0x0C);
        BUILD_BUG_ON(offsetof(compat_siginfo_t, si_syscall)   != 0x10);
-       BUILD_BUG_ON(offsetof(compat_siginfo_t, si_arch)      != 0x14);
+       BUILD_BUG_ON(offsetof(compat_siginfo_t, si_arch)      != 0x18);
 
        /* any new si_fields should be added here */
 }

the error message looks like this:

ld.lld: error: undefined symbol: BUILD_BUG_ON failed: offsetof(compat_siginfo_t, si_arch) != 0x18
>>> referenced by signal_compat.c
>>>               kernel/signal_compat.o:(sigaction_compat_abi) in archive arch/x86/built-in.a

Not perfect but at least you can see the error message. It does require LLVM_IAS=1 because GNU as doesn't like these weird symbol names. I don't know if the kernel folks will go for this though.

nickdesaulniers commented

I also have this diff to send to LKML:

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index cb9217fc60af..4ee87cbe7208 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -44,7 +44,6 @@
 #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 
 #define __compiletime_warning(message) __attribute__((__warning__(message)))
-#define __compiletime_error(message) __attribute__((__error__(message)))
 
 #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
 #define __latent_entropy __attribute__((latent_entropy))
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 2487be0e7199..fcdcb711d715 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -137,6 +137,12 @@
 # define __designated_init
 #endif
 
+#if __has_attribute(__error__)
+# define __compiletime_error(msg)       __attribute__((__error__(msg)))
+#else
+# define __compiletime_error(msg)
+#endif
+
 /*
  * Optional: not supported by clang
  *
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index e4ea86fc584d..cee45de0f61b 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -297,9 +297,6 @@ struct ftrace_likely_data {
 #ifndef __compiletime_warning
 # define __compiletime_warning(message)
 #endif
-#ifndef __compiletime_error
-# define __compiletime_error(message)
-#endif
 
 #ifdef __OPTIMIZE__
 # define __compiletime_assert(condition, msg, prefix, suffix)          \
nickdesaulniers commented

fixed in llvm-14:

  1. https://reviews.llvm.org/rG846e562dcc6a9a611d844dc0d123da95629a0567
  2. https://reviews.llvm.org/rGb72fd31bdaf2cba3bac03a1d83a231266160527c

Let's wait to close this out though until @ojeda picks up the kernel patch (with suggestion to s/clang-13/clang-14/) then that hits mainline.

kees commented

Awesome! I can confirm this works as expected for the FORTIFY_SOURCE warnings:
https://godbolt.org/z/qGrPqceWf

  • 👍2