[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230218020320.GA4163@brightrain.aerifal.cx>
Date: Fri, 17 Feb 2023 21:03:20 -0500
From: Rich Felker <dalias@...c.org>
To: Fangrui Song <i@...kray.me>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] Use __builtin_FILE/__builtin_LINE if available
On Fri, Feb 17, 2023 at 05:33:33PM -0800, Fangrui Song wrote:
> C++ inline functions are requred to have exact same sequence of tokens
> in every translation unit, but __FILE__ and __LINE__ may expand to
> different tokens. The ODR violatioin is usually benign, but it can lead
> to errors when C++20 modules are used.
>
> echo 'import B; import C; int main() { foo(); }' > A.cc
> cat > B.ccm <<'eof'
> module;
> #include <assert.h>
> export module B; export inline void foo() { assert(1); }
> eof
> cat > C.ccm <<'eof'
> module;
> #include <assert.h>
> export module C; export inline void foo() { assert(1); }
> eof
> clang -std=c++20 --precompile B.ccm -o B.pcm
> clang -std=c++20 --precompile C.ccm -o C.pcm
> clang -std=c++20 -fprebuilt-module-path=. A.cc B.pcm C.pcm -o A
>
> /tmp/d/C.ccm:3:37: error: 'foo' has different definitions in different modules; definition in module 'C' first difference is function body
> export module C; export inline void foo() { assert(1); }
> ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> /tmp/d/B.ccm:3:37: note: but in 'B' found a different body
> export module B; export inline void foo() { assert(1); }
> ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
>
> Fix this by preferring __builtin_FILE/__builtin_LINE which do not need
> preprocessing.
> ---
> include/assert.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/assert.h b/include/assert.h
> index d14ec94e..b209c2ae 100644
> --- a/include/assert.h
> +++ b/include/assert.h
> @@ -4,6 +4,12 @@
>
> #ifdef NDEBUG
> #define assert(x) (void)0
> +#elif defined(__has_builtin)
> +#if __has_builtin(__builtin_FILE)
> +#define assert(x) ((void)((x) || (__assert_fail(#x, __builtin_FILE(), __builtin_LINE(), __func__),0)))
> +#else
> +#define assert(x) ((void)((x) || (__assert_fail(#x, __FILE__, __LINE__, __func__),0)))
> +#endif
> #else
> #define assert(x) ((void)((x) || (__assert_fail(#x, __FILE__, __LINE__, __func__),0)))
> #endif
> --
> 2.39.GIT
It seems like use of assert here violates the ODR and is thus an
application error, no? In particular, it produces multiple definitions
that have differing behaviors, leaving which one actually gets used up
to the linker. Without the above change, LTO is able to diagnose the
error; with the change; it's silently deferred until runtime (where
the assertion violation message, of produced, will likely indicate the
wrong location).
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
