diff mbox

[rs6000] Fix many powerpc*-linux ASAN test suite failures

Message ID 1405464182.4285.24.camel@otta
State New
Headers show

Commit Message

Peter Bergner July 15, 2014, 10:43 p.m. UTC 
With a recent libsanitizer merge from upstream, we're now seeing a lot
of ASAN test suite failures with the following error:

  ==26426==ASan runtime does not come first in initial library list; you should
  either link runtime to your application or manually preload it with LD_PRELOAD.

This is caused by powerpc*-linux not defining LIBASAN_EARLY_SPEC which is
defined in gnu-user.h.  It looks like all *-linux architectures include
gnu-user.h except for powerpc*-linux.  The following patch makes powerpc*-linux
match the other linux architectures... and fixes a compiler error when we
try to redefine CC1_SPEC.

This passed bootstrap and regtesting on powerpc64-linux with no regressions.
Ok for mainline?

Peter

	* config.gcc (powerpc*-*-linux*): Include gnu-user.h in tm_file.
	* config/rs6000/sysv4.h (CC!_SPEC): Undefine it before defining it.

Comments

David Edelsohn July 16, 2014, 9:18 a.m. UTC | #1 
On Tue, Jul 15, 2014 at 6:43 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> With a recent libsanitizer merge from upstream, we're now seeing a lot
> of ASAN test suite failures with the following error:
>
>   ==26426==ASan runtime does not come first in initial library list; you should
>   either link runtime to your application or manually preload it with LD_PRELOAD.
>
> This is caused by powerpc*-linux not defining LIBASAN_EARLY_SPEC which is
> defined in gnu-user.h.  It looks like all *-linux architectures include
> gnu-user.h except for powerpc*-linux.  The following patch makes powerpc*-linux
> match the other linux architectures... and fixes a compiler error when we
> try to redefine CC1_SPEC.
>
> This passed bootstrap and regtesting on powerpc64-linux with no regressions.
> Ok for mainline?
>
> Peter
>
>         * config.gcc (powerpc*-*-linux*): Include gnu-user.h in tm_file.
>         * config/rs6000/sysv4.h (CC!_SPEC): Undefine it before defining it.

Typo in ChangeLog (CC!)?

This seems weird. Why wasn't this file included before or whenever it
was added for other *-linux targets?  This seems to define SPECs that
should have been necessary before now.

Thanks, David
Jakub Jelinek July 16, 2014, 9:23 a.m. UTC | #2 
On Wed, Jul 16, 2014 at 05:18:06AM -0400, David Edelsohn wrote:
> > This passed bootstrap and regtesting on powerpc64-linux with no regressions.
> > Ok for mainline?
> >
> > Peter
> >
> >         * config.gcc (powerpc*-*-linux*): Include gnu-user.h in tm_file.
> >         * config/rs6000/sysv4.h (CC!_SPEC): Undefine it before defining it.
> 
> Typo in ChangeLog (CC!)?
> 
> This seems weird. Why wasn't this file included before or whenever it
> was added for other *-linux targets?  This seems to define SPECs that
> should have been necessary before now.

All other Linux targets where asan is supported got the right definitions
from gnu-user.h, it was needed even on the older release branches.

As including gnu-user.h there might be too risky for the release branches,
perhaps it would be better to copy the LIB[AT]SAN* macros from gnu-user.h
to say rs6000/linux.h or rs6000/linux64.h on the release branches (and in 4.8 also
STATIC_LIB[AT]SAN_LIBS).

	Jakub
Peter Bergner July 16, 2014, 1:58 p.m. UTC | #3 
On Wed, 2014-07-16 at 11:23 +0200, Jakub Jelinek wrote:
> On Wed, Jul 16, 2014 at 05:18:06AM -0400, David Edelsohn wrote:
> > > This passed bootstrap and regtesting on powerpc64-linux with no regressions.
> > > Ok for mainline?
> > >
> > > Peter
> > >
> > >         * config.gcc (powerpc*-*-linux*): Include gnu-user.h in tm_file.
> > >         * config/rs6000/sysv4.h (CC!_SPEC): Undefine it before defining it.
> > 
> > Typo in ChangeLog (CC!)?

Woops, good catch.  I'll fix that.


> > This seems weird. Why wasn't this file included before or whenever it
> > was added for other *-linux targets?  This seems to define SPECs that
> > should have been necessary before now.

This was comitted by Joseph with revision 168711 and submitted here:

    https://gcc.gnu.org/ml/gcc-patches/2010-12/msg02055.html

The patch seems to move some defines from linux.h to gnu-user.h and
it looks like powerpc*-linux doesn't include linux.h either.
Is that another header file we're supposed to include?  ...or do
our rs6000/linux{,64}.h header files completely obviate the need
for that?


> All other Linux targets where asan is supported got the right definitions
> from gnu-user.h, it was needed even on the older release branches.
> 
> As including gnu-user.h there might be too risky for the release branches,
> perhaps it would be better to copy the LIB[AT]SAN* macros from gnu-user.h
> to say rs6000/linux.h or rs6000/linux64.h on the release branches (and in 4.8 also
> STATIC_LIB[AT]SAN_LIBS).

That's fine with me.  I'll make that change and bootstrap/regtest it.

Peter
Peter Bergner July 23, 2014, 8:06 p.m. UTC | #4 
On Wed, 2014-07-16 at 11:23 +0200, Jakub Jelinek wrote: 
> On Wed, Jul 16, 2014 at 05:18:06AM -0400, David Edelsohn wrote:
> > This seems weird. Why wasn't this file included before or whenever it
> > was added for other *-linux targets?  This seems to define SPECs that
> > should have been necessary before now.
> 
> All other Linux targets where asan is supported got the right definitions
> from gnu-user.h, it was needed even on the older release branches.

Ok, I compiled a source file that #includes tm.h and looked at the -dD -E
output to see what macros get defined with and without this change.
There are a few macros the gnu-user.h defines that are identical to what
some of the rs6000/sysv4.h or rs6000/linux{,64}.h header files define, so
we could remove those.  There are some that are different too, so we'd
need to keep those.  The following are the macros that are identical:

... in rs6000/sysv4.h are:

    LINK_EH_SPEC
    NO_IMPLICIT_EXTERN_C

However, there are some non-linux (eg, freebsd, lynx, etc.) that also
include rs6000/sysv4.h and not gnu-user.h, so I think we need to keep
this defines in rs6000/sysv4.h...or I could move them to freebsd{,64}.h,
lynx.h, etc.  Your preference is???

...in rs6000/linux{,64}.h are:

    ASM_APP_ON
    ASM_APP_OFF
    CPLUSPLUS_CPP_SPEC
    LINK_GCC_C_SEQUENCE_SPEC
    USE_LD_AS_NEEDED
    TARGET_POSIX_IO

These we should be able to freely remove from rs6000/linux{,64}.h since
gnu-user.h provides the same identical definitions.

There are also a few macros that gnu-user.h defines that are different than
the what rs6000/*.h files define (eg, STARTFILE_SPEC, ENDFILE_SPEC, CC1_SPEC
LIB_SPEC and TARGET_LIBC_HAS_FUNCTION), so we'll want to leave those macro
definitions as well.

Peter
Joseph Myers July 28, 2014, 5:27 p.m. UTC | #5 
On Wed, 16 Jul 2014, Peter Bergner wrote:

> > > This seems weird. Why wasn't this file included before or whenever it
> > > was added for other *-linux targets?  This seems to define SPECs that
> > > should have been necessary before now.
> 
> This was comitted by Joseph with revision 168711 and submitted here:
> 
>     https://gcc.gnu.org/ml/gcc-patches/2010-12/msg02055.html
> 
> The patch seems to move some defines from linux.h to gnu-user.h and
> it looks like powerpc*-linux doesn't include linux.h either.
> Is that another header file we're supposed to include?  ...or do
> our rs6000/linux{,64}.h header files completely obviate the need
> for that?

I believe the historical reason for not including the 
architecture-independent headers was the -mcall-* handling in 
rs6000/sysv4.h to allow a compiler for one target to build / link as if 
for another target for that architecture.  That arrangement means all the 
main specs are defined via per-OS named specs, with rs6000/sysv4.h 
containing the definitions of the underlying specs for each OS.  When 
specs come from the architecture-independent linux.h or gnu-user.h, other 
OS compilers with -mcall-linux will not get those specs and so will not 
properly link in the way a compiler configured for GNU/Linux target will.

Enough configuration is in fact done in the OS-specific headers that I 
don't think the -mcall-* handling of specs is useful any more (or has been 
for a long time).  (The compile-time handling of -mcall-* to select a 
particular ABI in SUBTARGET_OVERRIDE_OPTIONS may be more useful and seems 
unproblematic.)

So as I said in the message you refer to (and elsewhere), I think it would 
be a useful cleanup (and would not remove any feature that is actually 
usable) to remove the -mcall-* specs handling and make powerpc* targets 
more like other architectures in this regard - each OS's headers define 
the specs for that OS without the added indirection.

(I'm doubtful of the current use of -mads -myellowknife -mmvme as well - 
other architectures avoid having such special compiler options for each 
BSP.)
diff mbox

Patch

Index: gcc/config.gcc
===================================================================
--- gcc/config.gcc	(revision 212572)
+++ gcc/config.gcc	(working copy)
@@ -2243,7 +2243,7 @@  powerpc-*-rtems*)
 	tmake_file="${tmake_file} rs6000/t-fprules rs6000/t-rtems rs6000/t-ppccomm"
 	;;
 powerpc*-*-linux*)
-	tm_file="${tm_file} dbxelf.h elfos.h freebsd-spec.h rs6000/sysv4.h"
+	tm_file="${tm_file} dbxelf.h elfos.h gnu-user.h freebsd-spec.h rs6000/sysv4.h"
 	extra_options="${extra_options} rs6000/sysv4.opt"
 	tmake_file="rs6000/t-fprules rs6000/t-ppcos ${tmake_file} rs6000/t-ppccomm"
 	extra_objs="$extra_objs rs6000-linux.o"
Index: gcc/config/rs6000/sysv4.h
===================================================================
--- gcc/config/rs6000/sysv4.h	(revision 212572)
+++ gcc/config/rs6000/sysv4.h	(working copy)
@@ -539,6 +539,7 @@  ENDIAN_SELECT(" -mbig", " -mlittle", DEF
 #endif
 
 /* Pass -G xxx to the compiler.  */
+#undef CC1_SPEC
 #define	CC1_SPEC "%{G*} %(cc1_cpu)" \
 "%{meabi: %{!mcall-*: -mcall-sysv }} \
 %{!meabi: %{!mno-eabi: \