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

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

[Paul Pluzhnikov]

Greetings,

Attached patch changes assert in C++ mode from using __FILE__ and
__LINE__ to using __builtin_FILE() and __builtin_LINE().
These are supported by GCC since ~2012.

Motivation: C++20 modules.

When building C++, inline functions are required to have the exact
same sequence of tokens in every translation unit. But __FILE__ token,
when used in a header file, does not necessarily expand to the exact
same string literal.

For example:

// a.h
#include <assert.h>
inline void fn () { assert (0); }

// a.cc
#include "a.h"

// b.cc
#include "foo/../a.h"

Preprocessing a.cc and b.cc produces:
   (static_cast <bool> (0) ? void (0) : __assert_fail ("0", "a.h", 2,
__extension__ __PRETTY_FUNCTION__));
and
   (static_cast <bool> (0) ? void (0) : __assert_fail ("0",
"foo/../a.h", 2,  __extension__ __PRETTY_FUNCTION__));
respectively.

Without C++ modules, this results in unpredictable output, and
technically there is an ODR violation.

When C++ modules are involved, the mismatched sequence of tokens may
be diagnosed and result in a build failure.

P.S. I am only aware of the __FILE__ expansion causing problems, but I
am changing __LINE__ as well for symmetry.
-- 
Paul Pluzhnikov

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)                                                            \

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

[Florian Weimer]

* 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

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

[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))

??

No need for any version dependent and nonportable builtins.

Rich

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

[Rich Felker]

On Tue, Jan 24, 2023 at 06:10:20AM -0500, Rich Felker wrote:
> 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))
> 
> ??
> 
> No need for any version dependent and nonportable builtins.

N/m, this of course "works" but prints the wrong file (assert.h). I
don't suppose there's any way to fix that; at least I don't see one.

Rich

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

[Florian Weimer]

* 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?

Thanks,
Florian

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

[Rich Felker]

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

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

[Jonathan Wakely]

On Tue, 24 Jan 2023 at 11:38, Rich Felker <dalias@libc.org> wrote:
>
> 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

Added to GCC 4.8.0 in 2012. I don't know about 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.

It's not that simple though, the definition consists of the same
sequence of tokens, but can still result in incompatible definitions:

char* f()
{
  static char array[__builtin_LINE()];
  return array;
}

c.f. https://cplusplus.github.io/CWG/issues/2678.html

> >  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?

Yes, we should probably make a similar change.

> 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.

Good luck changing that.

> 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.)

There's plenty of code like this:

struct vector
{
  int* data;
  size_t len;
  int& operator[](int n)
  {
    assert(n < len);
    return data[len];
  }
};

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

[Florian Weimer]

* Jonathan Wakely:

>>>>> I think __builtin_FILE and __builtin_LINE are farily recent GCC/Clang
>
> Added to GCC 4.8.0 in 2012. I don't know about Clang.

Oh, I didn't realize it's been this long.

>> > __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.
>
> It's not that simple though, the definition consists of the same
> sequence of tokens, but can still result in incompatible definitions:
>
> char* f()
> {
>   static char array[__builtin_LINE()];
>   return array;
> }
>
> c.f. https://cplusplus.github.io/CWG/issues/2678.html

Ugh, I was worried about something like that.  And the location
functions need to be constexpr.

There's also the issue that <cassert> is defined in terms of ISO C,
and ISO C specifies that __FILE__ and __LINE__ must be used.  Is that
another defect?  But maybe the difference is not observable because
__FILE__ cannot be redefined?

Thanks,
Florian

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

[Jonathan Wakely]

On Tue, 24 Jan 2023 at 12:08, Florian Weimer <fweimer@redhat.com> wrote:
>
> * Jonathan Wakely:
>
> >>>>> I think __builtin_FILE and __builtin_LINE are farily recent GCC/Clang
> >
> > Added to GCC 4.8.0 in 2012. I don't know about Clang.
>
> Oh, I didn't realize it's been this long.
>
> >> > __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.
> >
> > It's not that simple though, the definition consists of the same
> > sequence of tokens, but can still result in incompatible definitions:
> >
> > char* f()
> > {
> >   static char array[__builtin_LINE()];
> >   return array;
> > }
> >
> > c.f. https://cplusplus.github.io/CWG/issues/2678.html
>
> Ugh, I was worried about something like that.  And the location
> functions need to be constexpr.

Yes. But that's not a problem for assert because it can't be used to
define objects or constants that would vary between TUs.

> There's also the issue that <cassert> is defined in terms of ISO C,
> and ISO C specifies that __FILE__ and __LINE__ must be used.  Is that
> another defect?  But maybe the difference is not observable because
> __FILE__ cannot be redefined?

I don't think it's observable. C requires the text printed by a failed
assert to include "the values of the preprocessing macros __FILE__ and
__LINE__ " but it doesn't require the actual tokens to appear in the
definition. The built-ins do return "the values" of those macros, so
that seems OK to me.

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

[Szabolcs Nagy]

The 01/24/2023 12:17, Jonathan Wakely via Libc-alpha wrote:
> On Tue, 24 Jan 2023 at 12:08, Florian Weimer <fweimer@redhat.com> wrote:
> > There's also the issue that <cassert> is defined in terms of ISO C,
> > and ISO C specifies that __FILE__ and __LINE__ must be used.  Is that
> > another defect?  But maybe the difference is not observable because
> > __FILE__ cannot be redefined?
> 
> I don't think it's observable. C requires the text printed by a failed
> assert to include "the values of the preprocessing macros __FILE__ and
> __LINE__ " but it doesn't require the actual tokens to appear in the
> definition. The built-ins do return "the values" of those macros, so
> that seems OK to me.

is that a real fix? the token sequence is the same in different
TUs with the builtins but the actual printed path is different,
so if the linker picks a definition at random then the produced
binary is not deterministic.

why is this better than just using __FILE__?

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

[Jonathan Wakely]

On Tue, 24 Jan 2023 at 15:08, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 01/24/2023 12:17, Jonathan Wakely via Libc-alpha wrote:
> > On Tue, 24 Jan 2023 at 12:08, Florian Weimer <fweimer@redhat.com> wrote:
> > > There's also the issue that <cassert> is defined in terms of ISO C,
> > > and ISO C specifies that __FILE__ and __LINE__ must be used.  Is that
> > > another defect?  But maybe the difference is not observable because
> > > __FILE__ cannot be redefined?
> >
> > I don't think it's observable. C requires the text printed by a failed
> > assert to include "the values of the preprocessing macros __FILE__ and
> > __LINE__ " but it doesn't require the actual tokens to appear in the
> > definition. The built-ins do return "the values" of those macros, so
> > that seems OK to me.
>
> is that a real fix? the token sequence is the same in different
> TUs with the builtins but the actual printed path is different,
> so if the linker picks a definition at random then the produced
> binary is not deterministic.

In practice, that only happens if you copy & paste the inline function
definition so it appears in two different TUs. If the inline function
is defined in a header, as in 99.9% of cases, then __FILE__ and
__builtin_FILE() both give you the name of that header, and the
problem never happens anyway. If you do weird things (duplicating the
code so it appears in two separate TUs without being included by a
common header) and the inline function isn't inlined, then yes, some
calls might appear to come from the "other" definition, showing the
file and line number of that definition. C'est la vie.

> why is this better than just using __FILE__?

The version with __FILE__ has undefined behaviour, and might not even
compile in a strict implementation of C++20 modules.

The version with __builtin_FILE is valid C++, so required to compile.
You might still get non-deterministic output if you duplicate the code
(instead of defining it in a header), but that's also possible if you
have a function that prints the value of rand() in an assertion
message.

Valid code that has non-deterministic behaviour in weird use cases is
better than code that has undefined behaviour in those cases.

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

[Paul Pluzhnikov]

On Tue, Jan 24, 2023 at 7:51 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Tue, 24 Jan 2023 at 15:08, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:

> > is that a real fix? the token sequence is the same in different
> > TUs with the builtins but the actual printed path is different,
> > so if the linker picks a definition at random then the produced
> > binary is not deterministic.

Any given binary is deterministic (the linker picks one or the other
version), but rebuilding this binary again after unrelated changes may
change the output, yes.

> In practice, that only happens if you copy & paste the inline function
> definition so it appears in two different TUs.

That doesn't seem correct.

As the original example shows, the same "a.h" can be #included
differently in different TUs, and either __FILE__ or __builtin_FILE()
will give different output in these TUs.

-- 
Paul Pluzhnikov

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

[Jonathan Wakely]

On Thu, 26 Jan 2023 at 19:18, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
> On Tue, Jan 24, 2023 at 7:51 AM Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On Tue, 24 Jan 2023 at 15:08, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> > > is that a real fix? the token sequence is the same in different
> > > TUs with the builtins but the actual printed path is different,
> > > so if the linker picks a definition at random then the produced
> > > binary is not deterministic.
>
> Any given binary is deterministic (the linker picks one or the other
> version), but rebuilding this binary again after unrelated changes may
> change the output, yes.
>
> > In practice, that only happens if you copy & paste the inline function
> > definition so it appears in two different TUs.
>
> That doesn't seem correct.
>
> As the original example shows, the same "a.h" can be #included
> differently in different TUs, and either __FILE__ or __builtin_FILE()
> will give different output in these TUs.


Ah yes, agreed (sorry, I was only "invited" to the thread half way
through). For __LINE__ both cases will give the same line, but the
include path causes __FILE__ to be different.

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

[Paul Pluzhnikov]

Ping?

Are there any strong objections to this patch which haven't been addressed?

It seems that
- it's a complete no-op in plain-C
- it helps C++ modules
- it has no real negatives

and as such should be a "no brainer".

Thanks,
-- 
Paul Pluzhnikov

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

[Florian Weimer]

* Paul Pluzhnikov:

> Are there any strong objections to this patch which haven't been addressed?
>
> It seems that
> - it's a complete no-op in plain-C
> - it helps C++ modules
> - it has no real negatives
>
> and as such should be a "no brainer".

As I said before, use of these builtins needs to be conditionalized.

Thanks,
Florian

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

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

On Sun, Feb 5, 2023 at 12:08 PM Florian Weimer <fweimer@redhat.com> wrote:

> As I said before, use of these builtins needs to be conditionalized.

Yes (revised patch attached).
I was more asking whether there were any fundamental objections.

Thanks,

--
Paul Pluzhnikov

[-- Attachment #2: assert-patch-20230205.txt --]
[-- Type: text/plain, Size: 940 bytes --]

diff --git a/assert/assert.h b/assert/assert.h
index 72209bc5e7..6edcb01d0a 100644
--- a/assert/assert.h
+++ b/assert/assert.h
@@ -86,10 +86,18 @@ __END_DECLS
    parentheses around EXPR.  Otherwise, those added parentheses would
    suppress warnings we'd expect to be detected by gcc's -Wparentheses.  */
 # if defined __cplusplus
+#  if __has_builtin(__builtin_FILE)
+#   define __ASSERT_FILE __builtin_FILE()
+#   define __ASSERT_LINE __builtin_LINE()
+#  else
+#   define __ASSERT_FILE __FILE__
+#   define __ASSERT_LINE __LINE__
+#  endif
 #  define assert(expr)							\
      (static_cast <bool> (expr)						\
       ? void (0)							\
-      : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
+      : __assert_fail (#expr, __ASSERT_FILE, __ASSERT_LINE,             \
+                       __ASSERT_FUNCTION))
 # elif !defined __GNUC__ || defined __STRICT_ANSI__
 #  define assert(expr)							\
     ((expr)								\

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

[Florian Weimer]

* Paul Pluzhnikov:

> +#  if __has_builtin(__builtin_FILE)
> +#   define __ASSERT_FILE __builtin_FILE()
> +#   define __ASSERT_LINE __builtin_LINE()
> +#  else
> +#   define __ASSERT_FILE __FILE__
> +#   define __ASSERT_LINE __LINE__
> +#  endif

__has_builtin itself needs to be conditionalized. 8-(

Please use __glibc_assert_file or something like that.  __ASSERT_FILE
sounds like something someone else might use (although it's in the
implementation namespace).

I think the general direction is fine.

Thanks,
Florian

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

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

On Sun, Feb 5, 2023 at 2:34 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Paul Pluzhnikov:
>
> > +#  if __has_builtin(__builtin_FILE)
> > +#   define __ASSERT_FILE __builtin_FILE()
> > +#   define __ASSERT_LINE __builtin_LINE()

I picked __ASSERT_FILE to match (already used) __ASSERT_FUNCTION.
Mixing __glibc_assert_file with __ASSERT_FUNCTION creates a cognitive
dissonance for me.

How strongly do you feel about this?

> __has_builtin itself needs to be conditionalized. 8-(

Revised patch attached.

Thanks,
-- 
Paul Pluzhnikov

[-- Attachment #2: assert-patch-20230205a.txt --]
[-- Type: text/plain, Size: 1016 bytes --]

diff --git a/assert/assert.h b/assert/assert.h
index 72209bc5e7..a28ad485e9 100644
--- a/assert/assert.h
+++ b/assert/assert.h
@@ -86,10 +86,21 @@ __END_DECLS
    parentheses around EXPR.  Otherwise, those added parentheses would
    suppress warnings we'd expect to be detected by gcc's -Wparentheses.  */
 # if defined __cplusplus
+#  if defined(__has_builtin)
+#   if __has_builtin(__builtin_FILE)
+#    define __ASSERT_FILE __builtin_FILE()
+#    define __ASSERT_LINE __builtin_LINE()
+#   endif
+#  endif
+#  if !defined(__ASSERT_FILE)
+#   define __ASSERT_FILE __FILE__
+#   define __ASSERT_LINE __LINE__
+#  endif
 #  define assert(expr)							\
      (static_cast <bool> (expr)						\
       ? void (0)							\
-      : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
+      : __assert_fail (#expr, __ASSERT_FILE, __ASSERT_LINE,             \
+                       __ASSERT_FUNCTION))
 # elif !defined __GNUC__ || defined __STRICT_ANSI__
 #  define assert(expr)							\
     ((expr)								\

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

[Florian Weimer]

* Paul Pluzhnikov:

> On Sun, Feb 5, 2023 at 2:34 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Paul Pluzhnikov:
>>
>> > +#  if __has_builtin(__builtin_FILE)
>> > +#   define __ASSERT_FILE __builtin_FILE()
>> > +#   define __ASSERT_LINE __builtin_LINE()
>
> I picked __ASSERT_FILE to match (already used) __ASSERT_FUNCTION.
> Mixing __glibc_assert_file with __ASSERT_FUNCTION creates a cognitive
> dissonance for me.
>
> How strongly do you feel about this?

Not strongly at all.  __ASSERT_FUNCTION is probably okay.

>> __has_builtin itself needs to be conditionalized. 8-(
>
> Revised patch attached.

Missing spaces before '(', sorry.  Rest looks okay if we can assume that
__builtin_FILE and __builtin_LINE are implemented at the same time.

Thanks,
Florian

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

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Sun, Feb 5, 2023 at 10:01 PM Florian Weimer <fweimer@redhat.com> wrote:

> Missing spaces before '(', sorry.

Done.

Rest looks okay if we can assume that
> __builtin_FILE and __builtin_LINE are implemented at the same time.

GCC has added these so long ago, we don't care (I think).
LLVM commit https://reviews.llvm.org/D37035 implemented both at the same time.

-- 
Paul Pluzhnikov

[-- Attachment #2: assert-patch-20230206.txt --]
[-- Type: text/plain, Size: 1018 bytes --]

diff --git a/assert/assert.h b/assert/assert.h
index 72209bc5e7..63197b819c 100644
--- a/assert/assert.h
+++ b/assert/assert.h
@@ -86,10 +86,21 @@ __END_DECLS
    parentheses around EXPR.  Otherwise, those added parentheses would
    suppress warnings we'd expect to be detected by gcc's -Wparentheses.  */
 # if defined __cplusplus
+#  if defined __has_builtin
+#   if __has_builtin (__builtin_FILE)
+#    define __ASSERT_FILE __builtin_FILE ()
+#    define __ASSERT_LINE __builtin_LINE ()
+#   endif
+#  endif
+#  if !defined(__ASSERT_FILE)
+#   define __ASSERT_FILE __FILE__
+#   define __ASSERT_LINE __LINE__
+#  endif
 #  define assert(expr)							\
      (static_cast <bool> (expr)						\
       ? void (0)							\
-      : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
+      : __assert_fail (#expr, __ASSERT_FILE, __ASSERT_LINE,             \
+                       __ASSERT_FUNCTION))
 # elif !defined __GNUC__ || defined __STRICT_ANSI__
 #  define assert(expr)							\
     ((expr)								\

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

[Paul Pluzhnikov]

On Mon, Feb 6, 2023 at 8:25 AM Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
> On Sun, Feb 5, 2023 at 10:01 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> > Missing spaces before '(', sorry.
>
> Done.

I intend to submit this patch on Friday if there are no additional comments.

Thanks,
-- 
Paul Pluzhnikov

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

[Paul Pluzhnikov]

On Tue, Jan 24, 2023 at 3:54 AM Jonathan Wakely <jwakely@redhat.com> wrote:

> Added to GCC 4.8.0 in 2012. I don't know about Clang.

(Like I said in the original email.)

Clang implementation is quite a bit more recent:

commit 708afb56c125ca4f7db7070e836860076c7eafbc
Author: Eric Fiselier <eric@efcs.ca>
Date:   Thu May 16 21:04:15 2019 +0000

    Implement __builtin_LINE() et. al. to support source location capture.


I feel like a __has_builtin gate isn't really necessary here, but will
put one in if people think it's necessary.

On Tue, 24 Jan 2023 at 11:38, Rich Felker <dalias@libc.org> wrote:

> (Also, C++ programmers should stop putting so much code in inline
> functions in headers.

There are C++ projects (Boost, Eigen) which consist _entirely_  of headers.
It was Eigen that has triggered this thread.

We could rewrite eigen_assert() to use source_location instead of
assert(), but I thought fixing it here is better, since it fixes all
C++ projects and not just Eigen.



--
Paul Pluzhnikov