Re: [musl] [PATCH] Use __builtin_FILE/__builtin_LINE if available

[Fangrui Song <i@maskray.me>]

On Fri, Feb 17, 2023 at 6:03 PM Rich Felker <dalias@libc.org> wrote:
>
> 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

I am unsure whether it is an application error in the above example.
If it is not a good example, here is another one
where the inline function using assert is in a header:


echo 'import B; import C; int main() { foo(); }' > A.cc
cat > a.h <<'eof'
#include <assert.h>
inline void fn() { assert(1); }
eof
cat > B.ccm <<'eof'
module;
#include "a.h"
export module B; export inline void foo() { fn(); }
eof
mkdir -p ./d
cat > d/C.ccm <<'eof'
module;
#include "../a.h"
export module C; export inline void foo() { fn(); }
eof
sed 's/^        /\t/' > Makefile <<'eof'
C := clang
all:
$C -std=c++20 --precompile B.ccm -o B.pcm
$C -std=c++20 --precompile d/C.ccm -o d/C.pcm
$C -std=c++20 -fprebuilt-module-path=. -fprebuilt-module-path=d A.cc
B.pcm d/C.pcm -o A
eof