[Bug libstdc++/105810] New: __glibcxx_assert can be improved greatly.

[Bug libstdc++/105810] New: __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

            Bug ID: 105810
           Summary: __glibcxx_assert can be improved greatly.
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: unlvsur at live dot com
  Target Milestone: ---

Every function call has to carry the call which adds a burden. With -Os the
function does not even get expanded. The operator[] becomes extremely
expensive.
https://godbolt.org/z/Gcchdrdxj

The situation might get worse when compiling with -fPIC since load/store data
from %rip register needs more instructions.

It can be improved to something like this. Put all the file, line, and
condition information into a separate function. Since the function is an inline
function, the entire program only holds one copy of the debugging information.
https://godbolt.org/z/xc8jnqh5r


https://godbolt.org/z/85M1PE9Te
vs
https://godbolt.org/z/6fsGo41bj


Unfortunately, GCC got ICE. I have submitted the bug report to fix that.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105809

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #1 from cqwrteur <unlvsur at live dot com> ---
You can see this generates extremely good assembly even under -Os
https://godbolt.org/z/oaerzYeo6

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Specifically, the suggested implementation is:

template<typename __glibcxxassertiontype>
[[noreturn,__gnu__::__cold__,__gnu__::__noinline__]]
inline  void __my_glibcxx_constexpr_assert() noexcept
{
    constexpr __glibcxxassertiontype __assertinfo;
   
__glibcxx_assert_fail(__assertinfo.__glibcxx_assertion_file,__assertinfo.__glibcxx_assertion_line,
       
__assertinfo.__glibcxx_pretty_function,__assertinfo.__glibcxx_assertion_condition);
}

#define my_glibcxx_assert(_Condition) \
  { \
  if (!bool(_Condition))[[unlikely]]    \
  {                                                     \
    \
    constexpr char const* __glibcxx_pretty_function_impl =
__PRETTY_FUNCTION__;\
    struct __glibcxxassertion{\
        char const* __glibcxx_assertion_file=__FILE__;\
        int __glibcxx_assertion_line=__LINE__;\
        char const* __glibcxx_pretty_function="";\
        char const* __glibcxx_assertion_condition=#_Condition;\
    };\
    __my_glibcxx_constexpr_assert<__glibcxxassertion>();                       
        \
  }\
  }


This instantiates a new instance of __my_glibcxx_constexpr_assert<T> with a new
T for every assertion, but the actual call to __glibcxx_assert_fail is in a
cold function instead of inlined into the assertion.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #3 from cqwrteur <unlvsur at live dot com> ---
(In reply to Jonathan Wakely from comment #2)
> Specifically, the suggested implementation is:
> 
> template<typename __glibcxxassertiontype>
> [[noreturn,__gnu__::__cold__,__gnu__::__noinline__]]
> inline  void __my_glibcxx_constexpr_assert() noexcept
> {
>     constexpr __glibcxxassertiontype __assertinfo;
>    
> __glibcxx_assert_fail(__assertinfo.__glibcxx_assertion_file,__assertinfo.
> __glibcxx_assertion_line,
>        
> __assertinfo.__glibcxx_pretty_function,__assertinfo.
> __glibcxx_assertion_condition);
> }
> 
> #define my_glibcxx_assert(_Condition) \
>   { \
>   if (!bool(_Condition))[[unlikely]]	\
>   {							\
>     \
>     constexpr char const* __glibcxx_pretty_function_impl =
> __PRETTY_FUNCTION__;\
>     struct __glibcxxassertion{\
>         char const* __glibcxx_assertion_file=__FILE__;\
>         int __glibcxx_assertion_line=__LINE__;\
>         char const* __glibcxx_pretty_function="";\
>         char const* __glibcxx_assertion_condition=#_Condition;\
>     };\
>     __my_glibcxx_constexpr_assert<__glibcxxassertion>();				\
>   }\
>   }
> 
> 
> This instantiates a new instance of __my_glibcxx_constexpr_assert<T> with a
> new T for every assertion, but the actual call to __glibcxx_assert_fail is
> in a cold function instead of inlined into the assertion.

yep.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #4 from cqwrteur <unlvsur at live dot com> ---
(In reply to cqwrteur from comment #3)
> (In reply to Jonathan Wakely from comment #2)
> > Specifically, the suggested implementation is:
> > 
> > template<typename __glibcxxassertiontype>
> > [[noreturn,__gnu__::__cold__,__gnu__::__noinline__]]
> > inline  void __my_glibcxx_constexpr_assert() noexcept
> > {
> >     constexpr __glibcxxassertiontype __assertinfo;
> >    
> > __glibcxx_assert_fail(__assertinfo.__glibcxx_assertion_file,__assertinfo.
> > __glibcxx_assertion_line,
> >        
> > __assertinfo.__glibcxx_pretty_function,__assertinfo.
> > __glibcxx_assertion_condition);
> > }
> > 
> > #define my_glibcxx_assert(_Condition) \
> >   { \
> >   if (!bool(_Condition))[[unlikely]]	\
> >   {							\
> >     \
> >     constexpr char const* __glibcxx_pretty_function_impl =
> > __PRETTY_FUNCTION__;\
> >     struct __glibcxxassertion{\
> >         char const* __glibcxx_assertion_file=__FILE__;\
> >         int __glibcxx_assertion_line=__LINE__;\
> >         char const* __glibcxx_pretty_function="";\
> >         char const* __glibcxx_assertion_condition=#_Condition;\
> >     };\
> >     __my_glibcxx_constexpr_assert<__glibcxxassertion>();				\
> >   }\
> >   }
> > 
> > 
> > This instantiates a new instance of __my_glibcxx_constexpr_assert<T> with a
> > new T for every assertion, but the actual call to __glibcxx_assert_fail is
> > in a cold function instead of inlined into the assertion.
> 
> yep.

btw since the function is inline linkaged, the entire program would only
contain one copy of __my_glibcxx_constexpr_assert<T>. For example for
std::span::operator[] the entire program shares the bounds checking part.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to cqwrteur from comment #4)
> btw since the function is inline linkaged, the entire program would only
> contain one copy of __my_glibcxx_constexpr_assert<T>. For example for
> std::span::operator[] the entire program shares the bounds checking part.

Isn't that already true today? span::operator[] is inline too, so there's only
one copy of that, and no copies of __my_glibcxx_constexpr_assert<T> at all.

You're adding lots of new symbols to the object files, which must be merged by
the linker. The difference is that setting up the arguments to
__glibcxx_assert_fail is in the new function, not in span::operator[].

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #6 from cqwrteur <unlvsur at live dot com> ---
(In reply to Jonathan Wakely from comment #5)
> (In reply to cqwrteur from comment #4)
> > btw since the function is inline linkaged, the entire program would only
> > contain one copy of __my_glibcxx_constexpr_assert<T>. For example for
> > std::span::operator[] the entire program shares the bounds checking part.
> 
> Isn't that already true today? span::operator[] is inline too, so there's
> only one copy of that, and no copies of __my_glibcxx_constexpr_assert<T> at
> all.
> 
> You're adding lots of new symbols to the object files, which must be merged
> by the linker. The difference is that setting up the arguments to
> __glibcxx_assert_fail is in the new function, not in span::operator[].

i mean the __my_glibcxx_constexpr_assert<T> part would be merged.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Right, but that function doesn't even exist today, so there's nothing to merge
today. This change adds new symbols (one for every assertion in the library)
which must be assembled and then merged by the linker.

I'm just not sure why you're pointing this out in comment 4 like it's an
advantage of the change. Of course there's only one copy of the function,
otherwise it would be an ODR violation, or a linker error. But it's still one
more function for every assertion in the library.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #8 from cqwrteur <unlvsur at live dot com> ---
(In reply to Jonathan Wakely from comment #7)
> Right, but that function doesn't even exist today, so there's nothing to
> merge today. This change adds new symbols (one for every assertion in the
> library) which must be assembled and then merged by the linker.
> 
> I'm just not sure why you're pointing this out in comment 4 like it's an
> advantage of the change. Of course there's only one copy of the function,
> otherwise it would be an ODR violation, or a linker error. But it's still
> one more function for every assertion in the library.

the thing is that.
if I ca(In reply to Jonathan Wakely from comment #7)
> Right, but that function doesn't even exist today, so there's nothing to
> merge today. This change adds new symbols (one for every assertion in the
> library) which must be assembled and then merged by the linker.
> 
> I'm just not sure why you're pointing this out in comment 4 like it's an
> advantage of the change. Of course there's only one copy of the function,
> otherwise it would be an ODR violation, or a linker error. But it's still
> one more function for every assertion in the library.

The thing is that:
https://godbolt.org/z/85M1PE9Te


The compiler has to generate
        subq    $8, %rsp
        leaq    .LC0(%rip), %rcx
        leaq    .LC1(%rip), %rdx
        movl    $278, %esi
        leaq    .LC2(%rip), %rdi
        call    std::__glibcxx_assert_fail(char const*, int, char const*, char
const*)@PLT

for every function when we use them.

while for the 2nd case 
https://godbolt.org/z/TjP7YxYcT

will only generate once for the assertion part.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #9 from cqwrteur <unlvsur at live dot com> ---
if I use std::span<T>, the compiler would have to emit assertions in every
function I use them, while the 2nd case would only do that once for the
operation [] case for example.
It reduces the code size of the function body, allowing more possibility for
inlining that particular function for example.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #10 from cqwrteur <unlvsur at live dot com> ---
(In reply to Jonathan Wakely from comment #7)
> Right, but that function doesn't even exist today, so there's nothing to
> merge today. This change adds new symbols (one for every assertion in the
> library) which must be assembled and then merged by the linker.
> 
> I'm just not sure why you're pointing this out in comment 4 like it's an
> advantage of the change. Of course there's only one copy of the function,
> otherwise it would be an ODR violation, or a linker error. But it's still
> one more function for every assertion in the library.

I am not saying "merging my code", I am saying the compiler would only emit
once. Not merging my code into libstdc++ something like that.

[Bug libstdc++/105810] __glibcxx_assert can be improved greatly.

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105810

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #2)
> Specifically, the suggested implementation is:
> 
> template<typename __glibcxxassertiontype>
> [[noreturn,__gnu__::__cold__,__gnu__::__noinline__]]
> inline  void __my_glibcxx_constexpr_assert() noexcept
> {
>     constexpr __glibcxxassertiontype __assertinfo;
>    
> __glibcxx_assert_fail(__assertinfo.__glibcxx_assertion_file,__assertinfo.
> __glibcxx_assertion_line,
>        
> __assertinfo.__glibcxx_pretty_function,__assertinfo.
> __glibcxx_assertion_condition);
> }
> 
> #define my_glibcxx_assert(_Condition) \
>   { \
>   if (!bool(_Condition))[[unlikely]]	\

N.B. we can't use [[unlikely]] in C++98 mode, it needs to be __builtin_expect.
We can't even use [[unlikely]] in C++11, it would need to be [[__unlikely__]].

>   {							\
>     \
>     constexpr char const* __glibcxx_pretty_function_impl =
> __PRETTY_FUNCTION__;\
>     struct __glibcxxassertion{\
>         char const* __glibcxx_assertion_file=__FILE__;\
>         int __glibcxx_assertion_line=__LINE__;\
>         char const* __glibcxx_pretty_function="";\
>         char const* __glibcxx_assertion_condition=#_Condition;\
>     };\
>     __my_glibcxx_constexpr_assert<__glibcxxassertion>();				\

We can't use a local class as a template argument in C++98, because the type
has no linkage.