Patchwork Committed: framework bits for disabling libsanitizer. RFC on which targets for which to disable it.

login
register
mail settings
Submitter Hans-Peter Nilsson
Date Nov. 13, 2012, 1:38 a.m.
Message ID <201211130138.qAD1cMWL013966@ignucius.se.axis.com>
Download mbox | patch
Permalink /patch/198518/
State New
Headers show

Comments

Hans-Peter Nilsson - Nov. 13, 2012, 1:38 a.m.
While the fallout(*) from the libsanitizer commit is handled,
it's obvious it should have a noconfigdirs= section in
toplevel/configure.ac like the other target libs.  Here's what I
committed after observing that a cris-elf build passed, where it
previously failed building libsanitizer which wrongly tries to
compile using -fPIC despite checking in its configure.ac IIUC.
But, I'd like to update the target contents there to something a
bit more generic.

Maybe disable libsanitizer everywhere except for (referring to
the three common target-related file-name parts in libsanitizer)
"mac", "win" and "linux"?  Or disable everywhere except x86_64 /
i386?  That's the only port that defines the required
TARGET_ASAN_SHADOW_OFFSET.  Maybe the library configure.ac
should check whether using -fsanitizer is error-free and disable
the libsanitizer build automatically?

toplevel:
	* configure.ac: Add section for noconfigdirs for libsanitizer.
	Disable for cris-*-* and mmix-*-*.
	* configure: Regenerate.



*) -fPIC passed when building libsanitizer for targets that
don't support it, lack of multilib setup for libsanitizer,
libsanitizer not installed in version-specific subdir...
Basically, IMHO it should just copy the generic
libgfortran/configure.ac and be done with it.  Right now it has
the smallest configure.ac of the target-libraries.

brgds, H-P
Dodji Seketeli - Nov. 13, 2012, 1:17 p.m.
Hans-Peter Nilsson <hans-peter.nilsson@axis.com> a écrit:

> While the fallout(*) from the libsanitizer commit is handled,
> it's obvious it should have a noconfigdirs= section in
> toplevel/configure.ac like the other target libs.  Here's what I
> committed after observing that a cris-elf build passed, where it
> previously failed building libsanitizer which wrongly tries to
> compile using -fPIC despite checking in its configure.ac IIUC.

Thank you for doing this.

> But, I'd like to update the target contents there to something a
> bit more generic.
>
> Maybe disable libsanitizer everywhere except for (referring to
> the three common target-related file-name parts in libsanitizer)
> "mac", "win" and "linux"?  Or disable everywhere except x86_64 /
> i386?  That's the only port that defines the required
> TARGET_ASAN_SHADOW_OFFSET.

FWIW, I'd think we should just disable it everywhere except on x86_64 /
i*86 for now, as these are the only targets that define
TARGET_ASAN_SHADOW_OFFSET.

What do the maintainers think?

[...]

> Index: configure.ac
> ===================================================================
> --- configure.ac	(revision 193455)
> +++ configure.ac	(working copy)
> @@ -547,6 +547,13 @@ case "${target}" in
>      ;;
>  esac
>  
> +# Disable libsanitizer for some systems.
> +case "${target}" in
> +  cris-*-* | crisv32-*-* | mmix-*-*)
> +    noconfigdirs="$noconfigdirs target-libsanitizer"
> +    ;;
> +esac
> +

[...]
Jakub Jelinek - Nov. 13, 2012, 1:24 p.m.
On Tue, Nov 13, 2012 at 02:17:55PM +0100, Dodji Seketeli wrote:
> What do the maintainers think?

Yes.  And it shouldn't be just based on target CPU, but also based
on target OS, I don't think libsanitizer supports anything but linux (glibc
+ maybe android) right now, with some smaller or bigger tweaks it could
support darwin (but see the reports that it doesn't build there right now)
or mingw/cygwin? (but there is a PR that it doesn't build there).
So IMHO it should be a whitelist of supported targets with *) case
adding noconfigdirs="$noconfigdirs target-libsanitizer", rather than
blacklist of few unsupported ones.  Can you please prepare a patch?

> > --- configure.ac	(revision 193455)
> > +++ configure.ac	(working copy)
> > @@ -547,6 +547,13 @@ case "${target}" in
> >      ;;
> >  esac
> >  
> > +# Disable libsanitizer for some systems.
> > +case "${target}" in
> > +  cris-*-* | crisv32-*-* | mmix-*-*)
> > +    noconfigdirs="$noconfigdirs target-libsanitizer"
> > +    ;;
> > +esac
> > +

	Jakub
Dodji Seketeli - Nov. 13, 2012, 2:06 p.m.
Jakub Jelinek <jakub@redhat.com> writes:

> On Tue, Nov 13, 2012 at 02:17:55PM +0100, Dodji Seketeli wrote:
>> What do the maintainers think?
>
> Yes.  And it shouldn't be just based on target CPU, but also based
> on target OS, I don't think libsanitizer supports anything but linux (glibc
> + maybe android) right now, with some smaller or bigger tweaks it could
> support darwin (but see the reports that it doesn't build there right now)
> or mingw/cygwin? (but there is a PR that it doesn't build there).
> So IMHO it should be a whitelist of supported targets with *) case
> adding noconfigdirs="$noconfigdirs target-libsanitizer", rather than
> blacklist of few unsupported ones.  Can you please prepare a patch?

Sure, will do.
Jack Howarth - Nov. 13, 2012, 2:53 p.m.
On Tue, Nov 13, 2012 at 03:06:56PM +0100, Dodji Seketeli wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> 
> > On Tue, Nov 13, 2012 at 02:17:55PM +0100, Dodji Seketeli wrote:
> >> What do the maintainers think?
> >
> > Yes.  And it shouldn't be just based on target CPU, but also based
> > on target OS, I don't think libsanitizer supports anything but linux (glibc
> > + maybe android) right now, with some smaller or bigger tweaks it could
> > support darwin (but see the reports that it doesn't build there right now)
> > or mingw/cygwin? (but there is a PR that it doesn't build there).
> > So IMHO it should be a whitelist of supported targets with *) case
> > adding noconfigdirs="$noconfigdirs target-libsanitizer", rather than
> > blacklist of few unsupported ones.  Can you please prepare a patch?
> 
> Sure, will do.
> 
> -- 
> 		Dodji

Dodji,
   I don't think darwin is very far from having usable asan support. Basically
we need the following changes...

1) The missing libsanitizer/interception/mach_override subdirectory with the files...

LICENSE.TXT	Makefile.mk	README.txt	mach_override.c	mach_override.h

needs to be imported from compiler-rt's git.

2) In libsanitizer/interception, mach_override/mach_override.c needs to be
added to interception_files and in libsanitizer/asan, the resulting object code
in libsanitizer/interception/mach_override/mach_override.o needs to be linked
into libasan.a and libasan.dylib.

3) In gcc/config/darwin.h, we need to add an entry to LINK_COMMAND_SPEC_A for
faddress-sanitizer to automatically pass -framework CoreFoundation -lasan to
the linker.
          Jack
Richard Henderson - Nov. 13, 2012, 8:38 p.m.
On 11/13/2012 05:24 AM, Jakub Jelinek wrote:
> Yes.  And it shouldn't be just based on target CPU, but also based
> on target OS, I don't think libsanitizer supports anything but linux (glibc
> + maybe android) right now, with some smaller or bigger tweaks it could
> support darwin (but see the reports that it doesn't build there right now)
> or mingw/cygwin? (but there is a PR that it doesn't build there).
> So IMHO it should be a whitelist of supported targets with *) case
> adding noconfigdirs="$noconfigdirs target-libsanitizer", rather than
> blacklist of few unsupported ones.  Can you please prepare a patch?

See how libatomic and libitm are structured.

The logic for what targets are supported belongs
inside the library directory, and not at top-level.

Add a configure.tgt script with that knowledge.


r~

Patch

Index: configure.ac
===================================================================
--- configure.ac	(revision 193455)
+++ configure.ac	(working copy)
@@ -547,6 +547,13 @@  case "${target}" in
     ;;
 esac
 
+# Disable libsanitizer for some systems.
+case "${target}" in
+  cris-*-* | crisv32-*-* | mmix-*-*)
+    noconfigdirs="$noconfigdirs target-libsanitizer"
+    ;;
+esac
+
 # Disable libssp for some systems.
 case "${target}" in
   avr-*-*)
Index: configure
===================================================================
--- configure	(revision 193455)
+++ configure	(working copy)
@@ -3205,6 +3205,13 @@  case "${target}" in
     ;;
 esac
 
+# Disable libsanitizer for some systems.
+case "${target}" in
+  cris-*-* | crisv32-*-* | mmix-*-*)
+    noconfigdirs="$noconfigdirs target-libsanitizer"
+    ;;
+esac
+
 # Disable libssp for some systems.
 case "${target}" in
   avr-*-*)