[B2/2] Use builtin_unreachable in assert

  Finally, use __builtin_unreachable to tell the compiler about
impossible paths.

The amount of code change with gcc 4.9.2 is enormous, and is usually
difficult to tell whether the change is definitely for the better.
Most of the time the difference begins with a branch test being reversed,
after which none of the remainder of the code lines up.

I did see one definitely positive case, gconv_builtin.os, which is
small enough to evaluate easily:

-  20:  48 8b 75 00             mov    0x0(%rbp),%rsi
-  24:  4c 89 f7                mov    %r14,%rdi
-  27:  e8 00 00 00 00          callq  2c <__gconv_get_builtin_trans+0x2c>
-                       28: R_X86_64_PLT32      __GI_strcmp-0x4
-  2c:  85 c0                   test   %eax,%eax
-  2e:  74 0e                   je     3e <__gconv_get_builtin_trans+0x3e>
-  30:  48 83 c3 01             add    $0x1,%rbx
-  34:  48 83 c5 20             add    $0x20,%rbp
-  38:  48 83 fb 0c             cmp    $0xc,%rbx
-  3c:  75 e2                   jne    20 <__gconv_get_builtin_trans+0x20>

+  20:  48 83 c5 01             add    $0x1,%rbp
+  24:  48 83 c3 20             add    $0x20,%rbx
+  28:  48 8b 33                mov    (%rbx),%rsi
+  2b:  4c 89 f7                mov    %r14,%rdi
+  2e:  e8 00 00 00 00          callq  33 <__gconv_get_builtin_trans+0x33>
+                       2f: R_X86_64_PLT32      __GI_strcmp-0x4
+  33:  85 c0                   test   %eax,%eax
+  35:  75 e9                   jne    20 <__gconv_get_builtin_trans+0x20>

The assert following the loop results in the elimination of the
loop comparison, leaving only the strcmp test controlling loop exit.

In leiu of investigaing all such, here's the final size comparison:

   text	   data	    bss	    dec	    hex	filename
1623772	  20968	  18160	1662900	 195fb4	bld-A1/libc.so
1623788	  20968	  18160	1662916	 195fc4	bld-B1/libc.so
1623604	  20968	  18160	1662732	 195f0c	bld-B2/libc.so


r~


PS: Note that __builtin_unreachable was introduced in gcc 4.5,
therefore no version test is required.


	* include/assert.h (assert): Use __builtin_unreachable.
	(assert_perror): Use __builtin_unreachable.
---
 include/assert.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On 09 Jan 2015 10:16, Richard Henderson wrote:
> Finally, use __builtin_unreachable to tell the compiler about
> impossible paths.

i'm not sure this is the way to go.  the expectation (as required by POSIX)
is that you're guaranteed there are no side-effects when NDEBUG is applied
to assert.  by using __builtin_unreachable, that is no longer the case.

yes, assert in this context is internal to glibc and so doesn't have to conform 
to POSIX directly.  but i think forcing developers to remember this kind of 
nuance just leads to bugs, especially when you consider glibc imports code from 
other projects (like gnulib).

the code shrinkage is attractive, but the other factors considered (and that 
NDEBUG usage in glibc in general is uncommon) leads me to think this path is 
not worth the pain.
-mike
  

On 03/01/2015 11:43 PM, Mike Frysinger wrote:
> On 09 Jan 2015 10:16, Richard Henderson wrote:
>> Finally, use __builtin_unreachable to tell the compiler about
>> impossible paths.
> 
> i'm not sure this is the way to go.  the expectation (as required by POSIX)
> is that you're guaranteed there are no side-effects when NDEBUG is applied
> to assert.  by using __builtin_unreachable, that is no longer the case.

I wonder if, during the gcc 6 development cycle, we should experiment with a
__builtin_side_effects_p, akin to __builtin_constant_p.  Then we could still
follow POSIX re no side effects but even in the external context provide the
information derivable from __builtin_unreachable.  I.e.

#define assert(X) \
  (__builtin_side_effects_p (X) || !(X) ? 0 : __builtin_unreachable ())



r~
  

On Mon, Mar 02, 2015 at 09:18:31AM -0800, Richard Henderson wrote:
> On 03/01/2015 11:43 PM, Mike Frysinger wrote:
> > On 09 Jan 2015 10:16, Richard Henderson wrote:
> >> Finally, use __builtin_unreachable to tell the compiler about
> >> impossible paths.
> > 
> > i'm not sure this is the way to go.  the expectation (as required by POSIX)
> > is that you're guaranteed there are no side-effects when NDEBUG is applied
> > to assert.  by using __builtin_unreachable, that is no longer the case.
> 
> I wonder if, during the gcc 6 development cycle, we should experiment with a
> __builtin_side_effects_p, akin to __builtin_constant_p.  Then we could still
> follow POSIX re no side effects but even in the external context provide the
> information derivable from __builtin_unreachable.  I.e.
> 
> #define assert(X) \
>   (__builtin_side_effects_p (X) || !(X) ? 0 : __builtin_unreachable ())

Tom wrote a patch implementing something like that a while back (for C
only) <https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01984.html>, but
the patch never made it in.  Maybe I could revive and adjust it during
gcc 6 stage 1 if it would help you.

	Marek
  

> i'm not sure this is the way to go.  the expectation (as required by POSIX)
> is that you're guaranteed there are no side-effects when NDEBUG is applied
> to assert.  by using __builtin_unreachable, that is no longer the case.

The requirement (which should be in ISO C, not in POSIX) is specified at
the C language level.  "No side effects" at the C level certainly does not
preclude a different correct translation of the program to machine code.

> the code shrinkage is attractive, but the other factors considered (and that 
> NDEBUG usage in glibc in general is uncommon) leads me to think this path is 
> not worth the pain.

In fact, many distributions build libc with -DNDEBUG.  In terms of quantity
of execution that happens, I'd bet most use of libc is with -DNDEBUG.


Thanks,
Roland
  

> I wonder if, during the gcc 6 development cycle, we should experiment with a
> __builtin_side_effects_p, akin to __builtin_constant_p.  Then we could still
> follow POSIX re no side effects but even in the external context provide the
> information derivable from __builtin_unreachable.  I.e.
> 
> #define assert(X) \
>   (__builtin_side_effects_p (X) || !(X) ? 0 : __builtin_unreachable ())

It would be ideal to do this in a way that some new warning option or
fortify enablement or whatnot can give compile-time warnings about side
effects in assert expressions.  How uselessly false-positivey that would be
depends on how much a particular codebase uses the style of data gathering
or consistency-checking function calls in assert expressions (i.e., things
that the compiler would consider to have side effects, perhaps even with
complete and perfect inlining, but the programmer reasonably considers to
have no actual semantic side effects).
  

On 02 Mar 2015 14:56, Roland McGrath wrote:
> > i'm not sure this is the way to go.  the expectation (as required by POSIX)
> > is that you're guaranteed there are no side-effects when NDEBUG is applied
> > to assert.  by using __builtin_unreachable, that is no longer the case.
> 
> The requirement (which should be in ISO C, not in POSIX) is specified at
> the C language level.

i don't have an ISO C reference, so i look at POSIX, and it explicitly defines 
it this way.

> "No side effects" at the C level certainly does not
> preclude a different correct translation of the program to machine code.

sure ... i wasn't saying otherwise.  my point is that __builtin_unreachable 
doesn't do that -- it explicitly is documented by gcc as:
  If control flow reaches the point of the __builtin_unreachable, the program is 
  undefined.

if there was a solution as Richard alluded to that allows for expansion of the 
expression all the time w/out breaking things when the assert is actually hit, 
i'm all for that.
-mike
  

> > "No side effects" at the C level certainly does not
> > preclude a different correct translation of the program to machine code.
> 
> sure ... i wasn't saying otherwise.  my point is that __builtin_unreachable 
> doesn't do that -- it explicitly is documented by gcc as:
>   If control flow reaches the point of the __builtin_unreachable, the program is 
>   undefined.
> 
> if there was a solution as Richard alluded to that allows for expansion of the 
> expression all the time w/out breaking things when the assert is actually hit, 
> i'm all for that.

I don't understand what your concern is.  A call to __assert_fail leads to
a call to abort and that is no more or less well-defined or recoverable
than __builtin_unreachable.
  

On 04 Mar 2015 11:25, Roland McGrath wrote:
> > > "No side effects" at the C level certainly does not
> > > preclude a different correct translation of the program to machine code.
> > 
> > sure ... i wasn't saying otherwise.  my point is that __builtin_unreachable 
> > doesn't do that -- it explicitly is documented by gcc as:
> >   If control flow reaches the point of the __builtin_unreachable, the program is 
> >   undefined.
> > 
> > if there was a solution as Richard alluded to that allows for expansion of the 
> > expression all the time w/out breaking things when the assert is actually hit, 
> > i'm all for that.
> 
> I don't understand what your concern is.  A call to __assert_fail leads to
> a call to abort and that is no more or less well-defined or recoverable
> than __builtin_unreachable.

yes, when NDEBUG is not defined.  but that's not what we're discussing here.

if your intention is for assert statements to crash the program even when NDEBUG 
is defined, then what exactly is the point of it ?  why don't we just delete it 
instead ?
-mike