Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve BUILD_BUG_ON error message with clang #1173

Closed
pcc opened this issue Oct 9, 2020 · 10 comments
Closed

Improve BUILD_BUG_ON error message with clang #1173

pcc opened this issue Oct 9, 2020 · 10 comments
Assignees
Labels
feature-request Not a bug per-se [FIXED][LINUX] 5.15 This bug was fixed in Linux 5.15 [FIXED][LLVM] 14 This bug was fixed in LLVM 14.x Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list.

Comments

@pcc
Copy link

pcc commented Oct 9, 2020

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 nickdesaulniers added the feature-request Not a bug per-se label Oct 14, 2020
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Oct 21, 2020

cc @arndb

@nickdesaulniers
Copy link
Member

nickdesaulniers commented Jan 21, 2021

See also: https://llvm.org/pr16428

@nickdesaulniers nickdesaulniers added the Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list. label Jan 21, 2021
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Jan 21, 2021

@nickdesaulniers
Copy link
Member

nickdesaulniers commented Jul 15, 2021

@nickdesaulniers nickdesaulniers self-assigned this Jul 15, 2021
@nickdesaulniers nickdesaulniers added the [PATCH] Submitted A patch has been submitted for review label Jul 15, 2021
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Jul 15, 2021

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
Copy link
Member

nickdesaulniers commented Jul 20, 2021

fengguang pushed a commit to 0day-ci/linux that referenced this issue Jul 20, 2021
I'm working on adding support for __attribute__((__error__(""))) and
__attribute__((__warning__(""))) to Clang. To make use of these in
__compiletime_error and __compiletime_warning (as used by BUILD_BUG and
friends) for newer clang and detect/fallback for older versions of
clang, move these to compiler_attributes.h and guard them with
__has_attribute preprocessor guards.

Link: https://reviews.llvm.org/D106030
Link: https://bugs.llvm.org/show_bug.cgi?id=16428
Link: ClangBuiltLinux#1173
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
fengguang pushed a commit to 0day-ci/linux that referenced this issue Aug 2, 2021
I'm working on adding support for __attribute__((__error__(""))) and
__attribute__((__warning__(""))) to Clang. To make use of these in
__compiletime_error and __compiletime_warning (as used by BUILD_BUG and
friends) for newer clang and detect/fallback for older versions of
clang, move these to compiler_attributes.h and guard them with
__has_attribute preprocessor guards.

Link: https://reviews.llvm.org/D106030
Link: https://bugs.llvm.org/show_bug.cgi?id=16428
Link: ClangBuiltLinux#1173
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Aug 4, 2021

nickdesaulniers added a commit to llvm/llvm-project that referenced this issue Aug 25, 2021
Add support for the GNU C style __attribute__((error(""))) and
__attribute__((warning(""))). These attributes are meant to be put on
declarations of functions whom should not be called.

They are frequently used to provide compile time diagnostics similar to
_Static_assert, but which may rely on non-ICE conditions (ie. relying on
compiler optimizations). This is also similar to diagnose_if function
attribute, but can diagnose after optimizations have been run.

While users may instead simply call undefined functions in such cases to
get a linkage failure from the linker, these provide a much more
ergonomic and actionable diagnostic to users and do so at compile time
rather than at link time. Users instead may be able use inline asm .err
directives.

These are used throughout the Linux kernel in its implementation of
BUILD_BUG and BUILD_BUG_ON macros. These macros generally cannot be
converted to use _Static_assert because many of the parameters are not
ICEs. The Linux kernel still needs to be modified to make use of these
when building with Clang; I have a patch that does so I will send once
this feature is landed.

To do so, we create a new IR level Function attribute, "dontcall" (both
error and warning boil down to one IR Fn Attr).  Then, similar to calls
to inline asm, we attach a !srcloc Metadata node to call sites of such
attributed callees.

The backend diagnoses these during instruction selection, while we still
know that a call is a call (vs say a JMP that's a tail call) in an arch
agnostic manner.

The frontend then reconstructs the SourceLocation from that Metadata,
and determines whether to emit an error or warning based on the callee's
attribute.

Link: https://bugs.llvm.org/show_bug.cgi?id=16428
Link: ClangBuiltLinux/linux#1173

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D106030
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Aug 25, 2021

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.

@nickdesaulniers nickdesaulniers added the [FIXED][LLVM] 14 This bug was fixed in LLVM 14.x label Aug 25, 2021
ojeda pushed a commit to ojeda/linux that referenced this issue Sep 9, 2021
Clang 14 will add support for __attribute__((__error__(""))) and
__attribute__((__warning__(""))). To make use of these in
__compiletime_error and __compiletime_warning (as used by BUILD_BUG and
friends) for newer clang and detect/fallback for older versions of
clang, move these to compiler_attributes.h and guard them with
__has_attribute preprocessor guards.

Link: https://reviews.llvm.org/D106030
Link: https://bugs.llvm.org/show_bug.cgi?id=16428
Link: ClangBuiltLinux#1173
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
[Reworded, landed in Clang 14]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Sep 14, 2021

b83a908
v5.15-rc1

@nickdesaulniers nickdesaulniers added [FIXED][LINUX] 5.15 This bug was fixed in Linux 5.15 and removed [PATCH] Submitted A patch has been submitted for review labels Sep 14, 2021
@kees
Copy link

kees commented Sep 15, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Not a bug per-se [FIXED][LINUX] 5.15 This bug was fixed in Linux 5.15 [FIXED][LLVM] 14 This bug was fixed in LLVM 14.x Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list.
Projects
None yet
Development

No branches or pull requests

3 participants