Re: [patch] Use __builtin_FILE and __builtin_LINE in assert implementation in C++

[Rich Felker <dalias@libc.org>]

On Tue, Jan 24, 2023 at 12:19:32PM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> > On Mon, Jan 23, 2023 at 03:26:04PM +0100, Florian Weimer via Libc-alpha wrote:
> >> * Paul Pluzhnikov via Libc-alpha:
> >> 
> >> > diff --git a/assert/assert.h b/assert/assert.h
> >> > index 72209bc5e7..4e0303db8d 100644
> >> > --- a/assert/assert.h
> >> > +++ b/assert/assert.h
> >> > @@ -89,7 +89,8 @@ __END_DECLS
> >> >  #  define assert(expr)                                                 \
> >> >       (static_cast <bool> (expr)
> >> >          \
> >> >        ? void (0)                                                       \
> >> > -      : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
> >> > +      : __assert_fail (#expr, __builtin_FILE (), __builtin_LINE (),     \
> >> > +                       __ASSERT_FUNCTION))
> >> >  # elif !defined __GNUC__ || defined __STRICT_ANSI__
> >> >  #  define assert(expr)                                                 \
> >> >      ((expr)                                                            \
> >> 
> >> I think __builtin_FILE and __builtin_LINE are farily recent GCC/Clang
> >> additions, so they need compiler version checks or a __has_builtin gate.
> >> 
> >> Ideally, we would use <source_location> here, but I don't think we can
> >> include that from <assert.h>.
> >> 
> >> Thanks,
> >> Florian
> >
> > Why not instead:
> >
> > +static char *__file_for_assert = __FILE__;
> > ...
> > -      : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
> > +      : __assert_fail (#expr, __file_for_assert, __LINE__, __ASSERT_FUNCTION))
> >
> > ??
> 
> __FILE__ expansion needs to be delayed, otherwise it refers to assert.h.
> 
> I think the builtins also have the advantage that they avoid ODR
> violations because the definition is the same no matter what the file is
> called.  This issue is not restricted to modules.  But maybe the right
> answer is to blame the programmer and just add a diagnostic?
> 
> Jonathan, I think the libstdc++ headers have the same issue.  Thoughts?

Arguably use of assert() in inline functions subject to this rule is a
programming error, since there's no guarantee it expands to the same
sequence of tokens as required. My leaning is that no action is
required here.

(Also, C++ programmers should stop putting so much code in inline
functions in headers. That's why C++ programs are so gigantic and take
ridiculous amounts of time to build. If it's big enough to merit
assertions it's almost surely too big to belong in an inline.)

Rich