Patchwork [rs6000] Enable ASAN build on powerpc*-linux

login
register
mail settings
Submitter Peter Bergner
Date Dec. 5, 2012, 10:34 p.m.
Message ID <1354746852.4743.16.camel@otta>
Download mbox | patch
Permalink /patch/204028/
State New
Headers show

Comments

Peter Bergner - Dec. 5, 2012, 10:34 p.m.
Below are the port changes to enable building ASAN on powerpc*-linux.
The libsanitizer changes required for powerpc*-linux have already been
committed.  This passes bootstrap and regtesting with no regressions
and we also pass the two ASAN test suite test cases.

Ok for mainline?

Peter


libsanitizer/
	* configure.tgt: Enable build on powerpc*-linux.

gcc/
	* config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
	(rs6000_asan_shadow_offset): New function.
	* config/rs6000/rs6000.h (FRAME_GROWS_DOWNWARD): Define to
	flag_stack_protect != 0 || flag_asan != 0.
David Edelsohn - Dec. 6, 2012, 2:39 p.m.
On Wed, Dec 5, 2012 at 5:34 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> Below are the port changes to enable building ASAN on powerpc*-linux.
> The libsanitizer changes required for powerpc*-linux have already been
> committed.  This passes bootstrap and regtesting with no regressions
> and we also pass the two ASAN test suite test cases.
>
> Ok for mainline?
>
> Peter
>
>
> libsanitizer/
>         * configure.tgt: Enable build on powerpc*-linux.
>
> gcc/
>         * config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
>         (rs6000_asan_shadow_offset): New function.
>         * config/rs6000/rs6000.h (FRAME_GROWS_DOWNWARD): Define to
>         flag_stack_protect != 0 || flag_asan != 0.

The definition of TARGET_ASAN_SHADOW_OFFSET and
rs6000_asan_shadow_offset need to be limited to TARGET_ELF or !
TARGET_XCOFF (Does it work on PPC BSD or PPC Darwin?). That macro
determines if GCC thinks -fsanitze=address is supported, which it is
not on AIX.  The macro probably should be defined in sysv4.h, not
rs6000.c, and the function should not be defined appropriately.

Thanks, David
Peter Bergner - Dec. 6, 2012, 2:46 p.m.
On Thu, 2012-12-06 at 09:39 -0500, David Edelsohn wrote:
> On Wed, Dec 5, 2012 at 5:34 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> >         * config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
> >         (rs6000_asan_shadow_offset): New function.
> >         * config/rs6000/rs6000.h (FRAME_GROWS_DOWNWARD): Define to
> >         flag_stack_protect != 0 || flag_asan != 0.
> 
> The definition of TARGET_ASAN_SHADOW_OFFSET and
> rs6000_asan_shadow_offset need to be limited to TARGET_ELF or !
> TARGET_XCOFF (Does it work on PPC BSD or PPC Darwin?).

That's a good question.  Having no way to test that myself, I
think limiting it to TARGET_ELF for now is the right solution
and if someone wants it to work on BSD or Darwin, they can
make the requisite changes and test it.


> That macro determines if GCC thinks -fsanitze=address is supported,
> which it is not on AIX.  The macro probably should be defined in sysv4.h,
> not rs6000.c, and the function should not be defined appropriately.

Ok, I can move the macro there.  And by the function being defined
appropriately, do you mean wrapping it in a:

  #ifdef TARGET_ASAN_SHADOW_OFFSET

or ???

Peter
Jakub Jelinek - Dec. 6, 2012, 2:53 p.m.
On Thu, Dec 06, 2012 at 08:46:20AM -0600, Peter Bergner wrote:
> On Thu, 2012-12-06 at 09:39 -0500, David Edelsohn wrote:
> > On Wed, Dec 5, 2012 at 5:34 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > >         * config/rs6000/rs6000.c (TARGET_ASAN_SHADOW_OFFSET): Define.
> > >         (rs6000_asan_shadow_offset): New function.
> > >         * config/rs6000/rs6000.h (FRAME_GROWS_DOWNWARD): Define to
> > >         flag_stack_protect != 0 || flag_asan != 0.
> > 
> > The definition of TARGET_ASAN_SHADOW_OFFSET and
> > rs6000_asan_shadow_offset need to be limited to TARGET_ELF or !
> > TARGET_XCOFF (Does it work on PPC BSD or PPC Darwin?).
> 
> That's a good question.  Having no way to test that myself, I
> think limiting it to TARGET_ELF for now is the right solution
> and if someone wants it to work on BSD or Darwin, they can
> make the requisite changes and test it.

On i?86/x86_64 it is provided for all targets, but only some of the targets
build libsanitizer then.  Either users get errors at compile time (missing
compiler support), or at link time (missing library support).
I think the compiler side should error out if the compiler can't support it
for some reason, if the only reason is missing library support, you should
get a link error.

> > That macro determines if GCC thinks -fsanitze=address is supported,
> > which it is not on AIX.  The macro probably should be defined in sysv4.h,
> > not rs6000.c, and the function should not be defined appropriately.
> 
> Ok, I can move the macro there.  And by the function being defined
> appropriately, do you mean wrapping it in a:
> 
>   #ifdef TARGET_ASAN_SHADOW_OFFSET
> 
> or ???

You can also provide the function always, and set
  targetm.asan_shadow_offset = NULL;
for AIX/etc. in some option post-processing target function (somewhere
before process_options will try to error out on missing asan support).

	Jakub
David Edelsohn - Dec. 6, 2012, 3:50 p.m.
On Thu, Dec 6, 2012 at 9:46 AM, Peter Bergner <bergner@vnet.ibm.com> wrote:

>> That macro determines if GCC thinks -fsanitze=address is supported,
>> which it is not on AIX.  The macro probably should be defined in sysv4.h,
>> not rs6000.c, and the function should not be defined appropriately.
>
> Ok, I can move the macro there.  And by the function being defined
> appropriately, do you mean wrapping it in a:
>
>   #ifdef TARGET_ASAN_SHADOW_OFFSET

I meant wrapping the code in

#if TARGET_ELF

or

#if !TARGET_XCOFF

or whatever is appropriate for where it is supported.

Thanks, David
David Edelsohn - Dec. 6, 2012, 3:55 p.m.
On Thu, Dec 6, 2012 at 9:53 AM, Jakub Jelinek <jakub@redhat.com> wrote:

> On i?86/x86_64 it is provided for all targets, but only some of the targets
> build libsanitizer then.  Either users get errors at compile time (missing
> compiler support), or at link time (missing library support).
> I think the compiler side should error out if the compiler can't support it
> for some reason, if the only reason is missing library support, you should
> get a link error.

This is my concern. If the library support is not present, I think it
is a worse user experience to fail with a link error because that does
not communicate to the user if the feature is supported or not.  The
library could be missing because it was not installed correctly or
some user error. And I think that it annoys the user to progress most
of the way through the build and fail later. If the compiler can know
that the feature is not supported, it should inform the user early and
with a message directly from the compiler.  The details of the reason
for lack of support are of interest to the GCC developer, not the end
user.

Thanks, David

Patch

Index: libsanitizer/configure.tgt
===================================================================
--- libsanitizer/configure.tgt	(revision 194226)
+++ libsanitizer/configure.tgt	(working copy)
@@ -25,6 +25,8 @@  case "${target}" in
 		TSAN_SUPPORTED=yes
 	fi
 	;;
+  powerpc*-*-linux*)
+	;;
   sparc*-*-linux*)
 	;;
   x86_64-*-darwin* | i?86-*-darwin*)
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 194226)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -1187,6 +1187,9 @@  static const struct attribute_spec rs600
 #undef TARGET_DELEGITIMIZE_ADDRESS
 #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
+
 #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
 #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
 
@@ -27521,6 +27524,13 @@  rs6000_final_prescan_insn (rtx insn, rtx
     }
 }
 
+/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
+
+static unsigned HOST_WIDE_INT
+rs6000_asan_shadow_offset (void)
+{
+  return (unsigned HOST_WIDE_INT) 1 << (TARGET_64BIT ? 41 : 29);
+}
 
 /* Mask options that we want to support inside of attribute((target)) and
    #pragma GCC target operations.  Note, we do not include things like
Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 194226)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -1406,7 +1406,7 @@  extern enum reg_class rs6000_constraints
 
    On the RS/6000, we grow upwards, from the area after the outgoing
    arguments.  */
-#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0)
+#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0 || flag_asan != 0)
 
 /* Size of the outgoing register save area */
 #define RS6000_REG_SAVE ((DEFAULT_ABI == ABI_AIX			\