soft-fp: Adjust call to abort for kernel use [committed]
diff mbox

Message ID alpine.DEB.2.10.1502201807380.30214@digraph.polyomino.org.uk
State New
Headers show

Commit Message

Joseph Myers Feb. 20, 2015, 6:07 p.m. UTC
soft-fp calls abort in various cases that the code doesn't handle, all
cases that should never actually occur for any supported choice of
types.

Calling an abort function is not appropriate for kernel use, so the
Linux kernel redefines abort as a macro in various ways in the ports
using this code, typically to "return 0" or similar.

One use of abort in soft-fp is inside a comma expression and doesn't
work with such a macro.  This patch changes it to use a statement
expression.

Tested for powerpc-nofpu that installed shared libraries are unchanged
by this patch.  Committed.

(There are two classes of aborts: those to make control flow visible
to the compiler, in default cases of switches over _FP_CLS_COMBINE,
which could reasonably change to __builtin_unreachable for glibc but
would still need to handle pre-4.5 compilers for kernel use, and those
intended to detect any use of soft-fp for combinations of types the
code doesn't know how to handle, which could reasonably become link
failures if the calls should always be optimized away.  But those are
separate possible future enhancements.)

2015-02-20  Joseph Myers  <joseph@codesourcery.com>

	* soft-fp/op-common.h (_FP_FROM_INT): Wrap call to abort in
	expression inside statement expression.

Comments

Rich Felker Feb. 20, 2015, 6:46 p.m. UTC | #1
On Fri, Feb 20, 2015 at 06:07:53PM +0000, Joseph Myers wrote:
> soft-fp calls abort in various cases that the code doesn't handle, all
> cases that should never actually occur for any supported choice of
> types.
> 
> Calling an abort function is not appropriate for kernel use, so the
> Linux kernel redefines abort as a macro in various ways in the ports
> using this code, typically to "return 0" or similar.
> 
> One use of abort in soft-fp is inside a comma expression and doesn't
> work with such a macro.  This patch changes it to use a statement
> expression.
> 
> Tested for powerpc-nofpu that installed shared libraries are unchanged
> by this patch.  Committed.
> 
> (There are two classes of aborts: those to make control flow visible
> to the compiler, in default cases of switches over _FP_CLS_COMBINE,
> which could reasonably change to __builtin_unreachable for glibc but
> would still need to handle pre-4.5 compilers for kernel use, and those
> intended to detect any use of soft-fp for combinations of types the
> code doesn't know how to handle, which could reasonably become link
> failures if the calls should always be optimized away.  But those are
> separate possible future enhancements.)
> 
> 2015-02-20  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* soft-fp/op-common.h (_FP_FROM_INT): Wrap call to abort in
> 	expression inside statement expression.
> 
> diff --git a/soft-fp/op-common.h b/soft-fp/op-common.h
> index 342532a..14fd6cd 100644
> --- a/soft-fp/op-common.h
> +++ b/soft-fp/op-common.h
> @@ -1815,7 +1815,7 @@
>  			 X##_e = (_FP_EXPBIAS_##fs + 2 * _FP_W_TYPE_SIZE - 1 \
>  				  - _FP_FROM_INT_lz);			\
>  		       })						\
> -		     : (abort (), 0)));					\
> +		     : ({ abort (); 0; })));				\
>  									\
>  	  if ((rsize) - 1 + _FP_EXPBIAS_##fs >= _FP_EXPMAX_##fs		\
>  	      && X##_e >= _FP_EXPMAX_##fs)				\

Could these be replaced by __builtin_trap() or similar? I'm not clear
on whether this code is purely glibc+kernel or also used by gcc, but
if it's used by gcc too it would be very much preferable not to have
libgcc pulling in a reference to abort (this affects -ffreestanding,
etc.).

Rich
David Miller Feb. 20, 2015, 6:58 p.m. UTC | #2
From: Rich Felker <dalias@libc.org>
Date: Fri, 20 Feb 2015 13:46:14 -0500

> Could these be replaced by __builtin_trap() or similar? I'm not clear
> on whether this code is purely glibc+kernel or also used by gcc, but
> if it's used by gcc too it would be very much preferable not to have
> libgcc pulling in a reference to abort (this affects -ffreestanding,
> etc.).

Another thing that occurs to me is that perhaps this can be coded
in such a way as to cause compile time errors.
Joseph Myers Feb. 20, 2015, 7 p.m. UTC | #3
On Fri, 20 Feb 2015, Rich Felker wrote:

> Could these be replaced by __builtin_trap() or similar? I'm not clear
> on whether this code is purely glibc+kernel or also used by gcc, but
> if it's used by gcc too it would be very much preferable not to have
> libgcc pulling in a reference to abort (this affects -ffreestanding,
> etc.).

This code is used by libgcc as well.  The particular case in this patch is 
one that could become a call to a nonexistent __softfp_link_failure or 
similar.  The ones that may sometimes end up as abort calls could be 
__builtin_unreachable.  There is never a need for an actual abort.

(Actually the aborts from _FP_CLS_COMBINE switches seem to get optimized 
out by current GCC anyway - I don't see them in the TFmode functions in 
libgcc from a recent x86_64 build, for example; looking at some old 
powerpc libraries I have to hand, I see them from GCC 4.3 but not 4.4.  
The ones in those switches are the only ones needing more than simple 
constant folding to optimize away.)
Joseph Myers Feb. 20, 2015, 9:31 p.m. UTC | #4
On Fri, 20 Feb 2015, David Miller wrote:

> From: Rich Felker <dalias@libc.org>
> Date: Fri, 20 Feb 2015 13:46:14 -0500
> 
> > Could these be replaced by __builtin_trap() or similar? I'm not clear
> > on whether this code is purely glibc+kernel or also used by gcc, but
> > if it's used by gcc too it would be very much preferable not to have
> > libgcc pulling in a reference to abort (this affects -ffreestanding,
> > etc.).
> 
> Another thing that occurs to me is that perhaps this can be coded
> in such a way as to cause compile time errors.

Static assertions (again, allowing for older GCC being used to compile the 
kernel) may well work for at least some of the cases where it's a matter 
of "soft-fp doesn't handle this possibility".  (Subject to making sure the 
relevant quantities are always constants; sometimes the kernel may use 
non-constant arguments even when glibc and libgcc always use constants.)

Patch
diff mbox

diff --git a/soft-fp/op-common.h b/soft-fp/op-common.h
index 342532a..14fd6cd 100644
--- a/soft-fp/op-common.h
+++ b/soft-fp/op-common.h
@@ -1815,7 +1815,7 @@ 
 			 X##_e = (_FP_EXPBIAS_##fs + 2 * _FP_W_TYPE_SIZE - 1 \
 				  - _FP_FROM_INT_lz);			\
 		       })						\
-		     : (abort (), 0)));					\
+		     : ({ abort (); 0; })));				\
 									\
 	  if ((rsize) - 1 + _FP_EXPBIAS_##fs >= _FP_EXPMAX_##fs		\
 	      && X##_e >= _FP_EXPMAX_##fs)				\