Patchwork PATCH: PR other/55292: libsanitizer doesn't support x32

login
register
mail settings
Submitter H.J. Lu
Date Nov. 12, 2012, 11:36 p.m.
Message ID <20121112233606.GA6375@gmail.com>
Download mbox | patch
Permalink /patch/198501/
State New
Headers show

Comments

H.J. Lu - Nov. 12, 2012, 11:36 p.m.
Hi,

Linux/x32 doesn't have __NR_mmap2/__NR_fstat64 and uses
__NR_mmap/__NR_fstat, just like Linux/x86-64.  Tested on Linux/x32.
OK to install?

Thanks.


H.J.
--
2012-11-12  H.J. Lu  <hongjiu.lu@intel.com>

	PR other/55292
	* sanitizer_common/sanitizer_linux.cc (internal_mmap): Use
	__NR_mmap if __x86_64__ is defined.
	(internal_filesize): Use __NR_fstat if __x86_64__ is defined.
Diego Novillo - Nov. 13, 2012, 2:02 a.m.
On Mon, Nov 12, 2012 at 6:36 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Hi,
>
> Linux/x32 doesn't have __NR_mmap2/__NR_fstat64 and uses
> __NR_mmap/__NR_fstat, just like Linux/x86-64.  Tested on Linux/x32.
> OK to install?

Patches to libsanitizer should be sent upstream.  We should only
contain a copy of the master in the LLVM repository.  There should be
instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
 I can't check ATM).


Thanks.  Diego.
H.J. Lu - Nov. 13, 2012, 2:16 a.m.
On Mon, Nov 12, 2012 at 6:02 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Mon, Nov 12, 2012 at 6:36 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> Hi,
>>
>> Linux/x32 doesn't have __NR_mmap2/__NR_fstat64 and uses
>> __NR_mmap/__NR_fstat, just like Linux/x86-64.  Tested on Linux/x32.
>> OK to install?
>
> Patches to libsanitizer should be sent upstream.  We should only
> contain a copy of the master in the LLVM repository.  There should be
> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>  I can't check ATM).
>

How does it work for

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291

AM_ENABLE_MULTILIB is used to provide multilib support for
GCC run-time library, which is only defined in config/multi.m4
in GCC source tree.  How do you generate configure using
AM_ENABLE_MULTILIB outside of GCC source tree?
Andrew Pinski - Nov. 13, 2012, 2:59 a.m.
On Mon, Nov 12, 2012 at 6:02 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Mon, Nov 12, 2012 at 6:36 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> Hi,
>>
>> Linux/x32 doesn't have __NR_mmap2/__NR_fstat64 and uses
>> __NR_mmap/__NR_fstat, just like Linux/x86-64.  Tested on Linux/x32.
>> OK to install?
>
> Patches to libsanitizer should be sent upstream.  We should only
> contain a copy of the master in the LLVM repository.  There should be
> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>  I can't check ATM).

I rather hate having to submit changes like this in two different
places.  Why can't the people who added the target library like this
take responsibility for doing the merges from the GCC source to the
upstream?  Like libffi is handled.

Thanks,
Andrew Pinski

>
>
> Thanks.  Diego.
H.J. Lu - Nov. 13, 2012, 6:56 a.m.
On Mon, Nov 12, 2012 at 6:59 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Nov 12, 2012 at 6:02 PM, Diego Novillo <dnovillo@google.com> wrote:
>> On Mon, Nov 12, 2012 at 6:36 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> Hi,
>>>
>>> Linux/x32 doesn't have __NR_mmap2/__NR_fstat64 and uses
>>> __NR_mmap/__NR_fstat, just like Linux/x86-64.  Tested on Linux/x32.
>>> OK to install?
>>
>> Patches to libsanitizer should be sent upstream.  We should only
>> contain a copy of the master in the LLVM repository.  There should be
>> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>  I can't check ATM).
>
> I rather hate having to submit changes like this in two different
> places.  Why can't the people who added the target library like this
> take responsibility for doing the merges from the GCC source to the
> upstream?  Like libffi is handled.
>

Agreed.  I created a git branch, hjl/asan, to address those issues.
It compiles/installs fine on Linux/x32 with lib, lib64 and libx32:

[hjl@gnu-tools-1 gcc-4.8.0-x32]$ file  */libasan*.0.0.0
lib64/libasan.so.0.0.0:  ELF 64-bit LSB shared object, x86-64, version
1 (GNU/Linux), dynamically linked,
BuildID[sha1]=0xd397454af874d82e45bffde03c18870588e9d901, not stripped
lib/libasan.so.0.0.0:    ELF 32-bit LSB shared object, Intel 80386,
version 1 (GNU/Linux), dynamically linked,
BuildID[sha1]=0x7b0c7b7793d3284cfad63f1a11bff1b814c3ac9a, not stripped
libx32/libasan.so.0.0.0: ELF 32-bit LSB shared object, x86-64, version
1 (GNU/Linux), dynamically linked,
BuildID[sha1]=0x699fa9a47f7cc24f9ff82c8663ecdf935b0f43a4, not stripped
[hjl@gnu-tools-1 gcc-4.8.0-x32]$

I am afraid that configure.ac change is very much specific to
compiling libsanitizer as a GCC target library.  Could someone
submit those changes upstream on behalf of GCC project?

Thanks.
Eric Botcazou - Nov. 13, 2012, 8:21 a.m.
> Patches to libsanitizer should be sent upstream.  We should only
> contain a copy of the master in the LLVM repository.  There should be
> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>  I can't check ATM).

I don't think that's acceptable.  GCC supports far more architectures and OSes 
than LLVM and thus requires additional and specific support code and we don't 
want to have to go through LLVM to change it.  As Andrew already suggested, 
this should be handled like libffi.
Dodji Seketeli - Nov. 13, 2012, 11:20 a.m.
Diego Novillo <dnovillo@google.com> a écrit:

> Patches to libsanitizer should be sent upstream.  We should only
> contain a copy of the master in the LLVM repository.  There should be
> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>  I can't check ATM).

No there are not, for the moment.  README.gcc just says where the
sources the 'upstream' project is.
H.J. Lu - Nov. 13, 2012, 11:31 a.m.
On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Diego Novillo <dnovillo@google.com> a écrit:
>
>> Patches to libsanitizer should be sent upstream.  We should only
>> contain a copy of the master in the LLVM repository.  There should be
>> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>  I can't check ATM).
>
> No there are not, for the moment.  README.gcc just says where the
> sources the 'upstream' project is.
>

What is the plan to add GCC specific support:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304

and

http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
Jakub Jelinek - Nov. 13, 2012, 11:42 a.m.
On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> > Diego Novillo <dnovillo@google.com> a écrit:
> >
> >> Patches to libsanitizer should be sent upstream.  We should only
> >> contain a copy of the master in the LLVM repository.  There should be
> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
> >>  I can't check ATM).
> >
> > No there are not, for the moment.  README.gcc just says where the
> > sources the 'upstream' project is.
> >
> 
> What is the plan to add GCC specific support:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
> 
> and
> 
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html

CCing Wei, I don't know the details about the import.  To me it looks like
that most or all of the libsanitizer/ level files (and
libsanitizer/*/Makefile.{am,in}) don't originate from
llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
should be changed to support multilibs, use the same libtool/autoconf/etc.
versions as rest of gcc etc.

For changes to the files actually imported from LLVM I guess it depends on
if google is going to accept such changes in the LLVM upstream.  For
unsupported targets we want to add target-libsanitizer into noconfigdirs
in toplevel configure.

Note that just making libsanitizer to build on some architecture is not
enough for full ASAN support, one needs to also add the target hook with
mem>>3 to shadow offset, and I guess review all other spots where
libsanitizer uses __i386__ or __x86_64__ macros.

I'd also say that using sanitizer_atomic_clang.h for GCC is not a good
idea, now that GCC 4.7+ has __atomic_* support that should be usable
for most of the __sanitizer::atomic* stuff.

	Jakub
Diego Novillo - Nov. 13, 2012, 1:26 p.m.
On Mon, Nov 12, 2012 at 9:59 PM, Andrew Pinski <pinskia@gmail.com> wrote:

> I rather hate having to submit changes like this in two different
> places.  Why can't the people who added the target library like this
> take responsibility for doing the merges from the GCC source to the
> upstream?  Like libffi is handled.

Hm, good point.  I had not considered that.  I think handling
libsanitizer patches similarly to what we do in libffi is a reasonable
approach.


Diego.
Konstantin Serebryany - Nov. 13, 2012, 9:40 p.m.
On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> > Diego Novillo <dnovillo@google.com> a écrit:
>> >
>> >> Patches to libsanitizer should be sent upstream.  We should only
>> >> contain a copy of the master in the LLVM repository.  There should be
>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>> >>  I can't check ATM).
>> >
>> > No there are not, for the moment.  README.gcc just says where the
>> > sources the 'upstream' project is.
>> >
>>
>> What is the plan to add GCC specific support:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>
>> and
>>
>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>
> CCing Wei, I don't know the details about the import.  To me it looks like
> that most or all of the libsanitizer/ level files (and
> libsanitizer/*/Makefile.{am,in}) don't originate from
> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
> should be changed to support multilibs, use the same libtool/autoconf/etc.
> versions as rest of gcc etc.


Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
files is a purely GCC thing.

>
> For changes to the files actually imported from LLVM I guess it depends on
> if google is going to accept such changes in the LLVM upstream.

Yes, we are willing to support any changes that make libasan support
more targets.
We would prefer all patches to go through LLVM first, and then ported
to GCC by copying files verbatim
This is the only way we can cope with the two versions.
(Wei, we will need the exact details for doing this in the README file)

--kcc


> For
> unsupported targets we want to add target-libsanitizer into noconfigdirs
> in toplevel configure.
>
> Note that just making libsanitizer to build on some architecture is not
> enough for full ASAN support, one needs to also add the target hook with
> mem>>3 to shadow offset, and I guess review all other spots where
> libsanitizer uses __i386__ or __x86_64__ macros.



>
> I'd also say that using sanitizer_atomic_clang.h for GCC is not a good
> idea, now that GCC 4.7+ has __atomic_* support that should be usable
> for most of the __sanitizer::atomic* stuff.
>
>         Jakub
Andrew Pinski - Nov. 13, 2012, 9:46 p.m.
On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>> >
>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>> >> contain a copy of the master in the LLVM repository.  There should be
>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>> >>  I can't check ATM).
>>> >
>>> > No there are not, for the moment.  README.gcc just says where the
>>> > sources the 'upstream' project is.
>>> >
>>>
>>> What is the plan to add GCC specific support:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>
>>> and
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>
>> CCing Wei, I don't know the details about the import.  To me it looks like
>> that most or all of the libsanitizer/ level files (and
>> libsanitizer/*/Makefile.{am,in}) don't originate from
>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>> versions as rest of gcc etc.
>
>
> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
> files is a purely GCC thing.
>
>>
>> For changes to the files actually imported from LLVM I guess it depends on
>> if google is going to accept such changes in the LLVM upstream.
>
> Yes, we are willing to support any changes that make libasan support
> more targets.
> We would prefer all patches to go through LLVM first, and then ported
> to GCC by copying files verbatim
> This is the only way we can cope with the two versions.
> (Wei, we will need the exact details for doing this in the README file)

I rather have it the other way around; like how libffi is handled.
Since GCC has many more targets and a different schedule than LLVM.

Thanks,
Andrew

>
> --kcc
>
>
>> For
>> unsupported targets we want to add target-libsanitizer into noconfigdirs
>> in toplevel configure.
>>
>> Note that just making libsanitizer to build on some architecture is not
>> enough for full ASAN support, one needs to also add the target hook with
>> mem>>3 to shadow offset, and I guess review all other spots where
>> libsanitizer uses __i386__ or __x86_64__ macros.
>
>
>
>>
>> I'd also say that using sanitizer_atomic_clang.h for GCC is not a good
>> idea, now that GCC 4.7+ has __atomic_* support that should be usable
>> for most of the __sanitizer::atomic* stuff.
>>
>>         Jakub
H.J. Lu - Nov. 13, 2012, 9:53 p.m.
On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>> >
>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>> >> contain a copy of the master in the LLVM repository.  There should be
>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>> >>  I can't check ATM).
>>> >
>>> > No there are not, for the moment.  README.gcc just says where the
>>> > sources the 'upstream' project is.
>>> >
>>>
>>> What is the plan to add GCC specific support:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>
>>> and
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>
>> CCing Wei, I don't know the details about the import.  To me it looks like
>> that most or all of the libsanitizer/ level files (and
>> libsanitizer/*/Makefile.{am,in}) don't originate from
>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>> versions as rest of gcc etc.
>
>
> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
> files is a purely GCC thing.
>
>>
>> For changes to the files actually imported from LLVM I guess it depends on
>> if google is going to accept such changes in the LLVM upstream.
>
> Yes, we are willing to support any changes that make libasan support
> more targets.
> We would prefer all patches to go through LLVM first, and then ported
> to GCC by copying files verbatim
> This is the only way we can cope with the two versions.
> (Wei, we will need the exact details for doing this in the README file)
>

Could someone please check this patch:

http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html

into upstream?

Thanks.
Xinliang David Li - Nov. 13, 2012, 10:13 p.m.
On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Diego Novillo <dnovillo@google.com> a écrit:
>
>> Patches to libsanitizer should be sent upstream.  We should only
>> contain a copy of the master in the LLVM repository.  There should be
>> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>  I can't check ATM).
>
> No there are not, for the moment.  README.gcc just says where the
> sources the 'upstream' project is.
>

Right -- and as it shows, this is not a simple matter and requires
joint wisdom from the community  :).

David



> --
>                 Dodji
Konstantin Serebryany - Nov. 13, 2012, 10:15 p.m.
On Tue, Nov 13, 2012 at 1:46 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>>> >
>>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>>> >> contain a copy of the master in the LLVM repository.  There should be
>>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>>> >>  I can't check ATM).
>>>> >
>>>> > No there are not, for the moment.  README.gcc just says where the
>>>> > sources the 'upstream' project is.
>>>> >
>>>>
>>>> What is the plan to add GCC specific support:
>>>>
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>>
>>>> and
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>>
>>> CCing Wei, I don't know the details about the import.  To me it looks like
>>> that most or all of the libsanitizer/ level files (and
>>> libsanitizer/*/Makefile.{am,in}) don't originate from
>>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>>> versions as rest of gcc etc.
>>
>>
>> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
>> files is a purely GCC thing.
>>
>>>
>>> For changes to the files actually imported from LLVM I guess it depends on
>>> if google is going to accept such changes in the LLVM upstream.
>>
>> Yes, we are willing to support any changes that make libasan support
>> more targets.
>> We would prefer all patches to go through LLVM first, and then ported
>> to GCC by copying files verbatim
>> This is the only way we can cope with the two versions.
>> (Wei, we will need the exact details for doing this in the README file)
>
> I rather have it the other way around; like how libffi is handled.
> Since GCC has many more targets and a different schedule than LLVM.

That may work too, but the very moment that the two versions get out of sync
we lose the ability to port the new version from LLVM to GCC with
reasonable effort.
So, whoever applies a change to the gcc version will need to make sure
the same change applies upstream.

>
> Thanks,
> Andrew
>
>>
>> --kcc
>>
>>
>>> For
>>> unsupported targets we want to add target-libsanitizer into noconfigdirs
>>> in toplevel configure.
>>>
>>> Note that just making libsanitizer to build on some architecture is not
>>> enough for full ASAN support, one needs to also add the target hook with
>>> mem>>3 to shadow offset, and I guess review all other spots where
>>> libsanitizer uses __i386__ or __x86_64__ macros.
>>
>>
>>
>>>
>>> I'd also say that using sanitizer_atomic_clang.h for GCC is not a good
>>> idea, now that GCC 4.7+ has __atomic_* support that should be usable
>>> for most of the __sanitizer::atomic* stuff.
>>>
>>>         Jakub
Konstantin Serebryany - Nov. 13, 2012, 10:15 p.m.
On Tue, Nov 13, 2012 at 1:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>>> >
>>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>>> >> contain a copy of the master in the LLVM repository.  There should be
>>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>>> >>  I can't check ATM).
>>>> >
>>>> > No there are not, for the moment.  README.gcc just says where the
>>>> > sources the 'upstream' project is.
>>>> >
>>>>
>>>> What is the plan to add GCC specific support:
>>>>
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>>
>>>> and
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>>
>>> CCing Wei, I don't know the details about the import.  To me it looks like
>>> that most or all of the libsanitizer/ level files (and
>>> libsanitizer/*/Makefile.{am,in}) don't originate from
>>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>>> versions as rest of gcc etc.
>>
>>
>> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
>> files is a purely GCC thing.
>>
>>>
>>> For changes to the files actually imported from LLVM I guess it depends on
>>> if google is going to accept such changes in the LLVM upstream.
>>
>> Yes, we are willing to support any changes that make libasan support
>> more targets.
>> We would prefer all patches to go through LLVM first, and then ported
>> to GCC by copying files verbatim
>> This is the only way we can cope with the two versions.
>> (Wei, we will need the exact details for doing this in the README file)
>>
>
> Could someone please check this patch:
>
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html
>
> into upstream?

Let me do this.

--kcc


>
> Thanks.
>
>
> --
> H.J.
Xinliang David Li - Nov. 13, 2012, 10:19 p.m.
On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> > Diego Novillo <dnovillo@google.com> a écrit:
>> >
>> >> Patches to libsanitizer should be sent upstream.  We should only
>> >> contain a copy of the master in the LLVM repository.  There should be
>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>> >>  I can't check ATM).
>> >
>> > No there are not, for the moment.  README.gcc just says where the
>> > sources the 'upstream' project is.
>> >
>>
>> What is the plan to add GCC specific support:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>
>> and
>>
>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>
> CCing Wei, I don't know the details about the import.  To me it looks like
> that most or all of the libsanitizer/ level files (and
> libsanitizer/*/Makefile.{am,in}) don't originate from
> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
> should be changed to support multilibs, use the same libtool/autoconf/etc.
> versions as rest of gcc etc.
>

Agree.


> For changes to the files actually imported from LLVM I guess it depends on
> if google is going to accept such changes in the LLVM upstream.  For
> unsupported targets we want to add target-libsanitizer into noconfigdirs
> in toplevel configure.
>

These would be files under libsanitizer/asan, libsanitizer/tsan,
libsanitizer/sanitizer_common, libsanitizer/include directories.

For changes in those directories, why not sending the patch to Kosyta
and Dmitry, whom I assume will help review the patch and do the commit
properly?

> Note that just making libsanitizer to build on some architecture is not
> enough for full ASAN support, one needs to also add the target hook with
> mem>>3 to shadow offset, and I guess review all other spots where
> libsanitizer uses __i386__ or __x86_64__ macros.
>
> I'd also say that using sanitizer_atomic_clang.h for GCC is not a good
> idea, now that GCC 4.7+ has __atomic_* support that should be usable
> for most of the __sanitizer::atomic* stuff.

Right, but that can be changed.

thanks,

David


>
>         Jakub
Konstantin Serebryany - Nov. 13, 2012, 10:26 p.m.
H.J.,
Question about this patch.
Will it work if we simply replace
   #if __WORDSIZE == 64
with
  #ifdef x86_64
?

Today, x86_64 is the only 64-bit architecture supported by asan
run-time on linux anyway.


On Tue, Nov 13, 2012 at 1:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>>> >
>>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>>> >> contain a copy of the master in the LLVM repository.  There should be
>>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>>> >>  I can't check ATM).
>>>> >
>>>> > No there are not, for the moment.  README.gcc just says where the
>>>> > sources the 'upstream' project is.
>>>> >
>>>>
>>>> What is the plan to add GCC specific support:
>>>>
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>>
>>>> and
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>>
>>> CCing Wei, I don't know the details about the import.  To me it looks like
>>> that most or all of the libsanitizer/ level files (and
>>> libsanitizer/*/Makefile.{am,in}) don't originate from
>>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>>> versions as rest of gcc etc.
>>
>>
>> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
>> files is a purely GCC thing.
>>
>>>
>>> For changes to the files actually imported from LLVM I guess it depends on
>>> if google is going to accept such changes in the LLVM upstream.
>>
>> Yes, we are willing to support any changes that make libasan support
>> more targets.
>> We would prefer all patches to go through LLVM first, and then ported
>> to GCC by copying files verbatim
>> This is the only way we can cope with the two versions.
>> (Wei, we will need the exact details for doing this in the README file)
>>
>
> Could someone please check this patch:
>
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html
>
> into upstream?
>
> Thanks.
>
>
> --
> H.J.
Andrew Pinski - Nov. 13, 2012, 10:29 p.m.
On Tue, Nov 13, 2012 at 2:26 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> H.J.,
> Question about this patch.
> Will it work if we simply replace
>    #if __WORDSIZE == 64
> with
>   #ifdef x86_64
> ?
>
> Today, x86_64 is the only 64-bit architecture supported by asan
> run-time on linux anyway.

Because x86_64 is defined even for x32.  And it is the only one
currently supported does not mean there will be more in the future.

Thanks,
Andrew Pinski

>
>
> On Tue, Nov 13, 2012 at 1:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>>>> >
>>>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>>>> >> contain a copy of the master in the LLVM repository.  There should be
>>>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>>>> >>  I can't check ATM).
>>>>> >
>>>>> > No there are not, for the moment.  README.gcc just says where the
>>>>> > sources the 'upstream' project is.
>>>>> >
>>>>>
>>>>> What is the plan to add GCC specific support:
>>>>>
>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>>>
>>>>> and
>>>>>
>>>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>>>
>>>> CCing Wei, I don't know the details about the import.  To me it looks like
>>>> that most or all of the libsanitizer/ level files (and
>>>> libsanitizer/*/Makefile.{am,in}) don't originate from
>>>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>>>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>>>> versions as rest of gcc etc.
>>>
>>>
>>> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
>>> files is a purely GCC thing.
>>>
>>>>
>>>> For changes to the files actually imported from LLVM I guess it depends on
>>>> if google is going to accept such changes in the LLVM upstream.
>>>
>>> Yes, we are willing to support any changes that make libasan support
>>> more targets.
>>> We would prefer all patches to go through LLVM first, and then ported
>>> to GCC by copying files verbatim
>>> This is the only way we can cope with the two versions.
>>> (Wei, we will need the exact details for doing this in the README file)
>>>
>>
>> Could someone please check this patch:
>>
>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html
>>
>> into upstream?
>>
>> Thanks.
>>
>>
>> --
>> H.J.
Konstantin Serebryany - Nov. 13, 2012, 10:34 p.m.
On Tue, Nov 13, 2012 at 2:29 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 2:26 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> H.J.,
>> Question about this patch.
>> Will it work if we simply replace
>>    #if __WORDSIZE == 64
>> with
>>   #ifdef x86_64
>> ?
>>
>> Today, x86_64 is the only 64-bit architecture supported by asan
>> run-time on linux anyway.
>
> Because x86_64 is defined even for x32.

Sure, this is why I suggest to use #if defined x86_64 instead of
#if __WORDSIZE == 64 || defined __x86_64__

>> And it is the only one
> currently supported does not mean there will be more in the future.

I hope there will be more, this is why #if defined x86_64 might be
preferable --
the code will explicitly be x86_64-specific and we'll know what to fix.

??

--kcc

>
> Thanks,
> Andrew Pinski
>
>>
>>
>> On Tue, Nov 13, 2012 at 1:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Nov 13, 2012 at 1:40 PM, Konstantin Serebryany
>>> <konstantin.s.serebryany@gmail.com> wrote:
>>>> On Tue, Nov 13, 2012 at 3:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> On Tue, Nov 13, 2012 at 03:31:21AM -0800, H.J. Lu wrote:
>>>>>> On Tue, Nov 13, 2012 at 3:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>>>>> > Diego Novillo <dnovillo@google.com> a écrit:
>>>>>> >
>>>>>> >> Patches to libsanitizer should be sent upstream.  We should only
>>>>>> >> contain a copy of the master in the LLVM repository.  There should be
>>>>>> >> instructions in libsanitizer/README.gcc (Jakub, Dodji, are they there?
>>>>>> >>  I can't check ATM).
>>>>>> >
>>>>>> > No there are not, for the moment.  README.gcc just says where the
>>>>>> > sources the 'upstream' project is.
>>>>>> >
>>>>>>
>>>>>> What is the plan to add GCC specific support:
>>>>>>
>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55291
>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55292
>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55304
>>>>>>
>>>>>> and
>>>>>>
>>>>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00967.html
>>>>>
>>>>> CCing Wei, I don't know the details about the import.  To me it looks like
>>>>> that most or all of the libsanitizer/ level files (and
>>>>> libsanitizer/*/Makefile.{am,in}) don't originate from
>>>>> llvm/projects/compiler-rt/lib , so they should be owned by GCC and thus
>>>>> should be changed to support multilibs, use the same libtool/autoconf/etc.
>>>>> versions as rest of gcc etc.
>>>>
>>>>
>>>> Correct. Whatever happens to Makefile, configure and other non-.{cc,h}
>>>> files is a purely GCC thing.
>>>>
>>>>>
>>>>> For changes to the files actually imported from LLVM I guess it depends on
>>>>> if google is going to accept such changes in the LLVM upstream.
>>>>
>>>> Yes, we are willing to support any changes that make libasan support
>>>> more targets.
>>>> We would prefer all patches to go through LLVM first, and then ported
>>>> to GCC by copying files verbatim
>>>> This is the only way we can cope with the two versions.
>>>> (Wei, we will need the exact details for doing this in the README file)
>>>>
>>>
>>> Could someone please check this patch:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html
>>>
>>> into upstream?
>>>
>>> Thanks.
>>>
>>>
>>> --
>>> H.J.
H.J. Lu - Nov. 13, 2012, 10:39 p.m.
On Tue, Nov 13, 2012 at 2:26 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> H.J.,
> Question about this patch.
> Will it work if we simply replace
>    #if __WORDSIZE == 64
> with
>   #ifdef x86_64
> ?
>
> Today, x86_64 is the only 64-bit architecture supported by asan
> run-time on linux anyway.
>
>

That works for me.

Thanks.
H.J. Lu - Nov. 13, 2012, 10:40 p.m.
On Tue, Nov 13, 2012 at 2:39 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 2:26 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> H.J.,
>> Question about this patch.
>> Will it work if we simply replace
>>    #if __WORDSIZE == 64
>> with
>>   #ifdef x86_64
>> ?
>>
>> Today, x86_64 is the only 64-bit architecture supported by asan
>> run-time on linux anyway.
>>
>>
>
> That works for me.
>

It should be

#ifdef __x86_64__

not

#ifdef x86_64
Konstantin Serebryany - Nov. 13, 2012, 11:14 p.m.
On Tue, Nov 13, 2012 at 2:40 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Nov 13, 2012 at 2:39 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Nov 13, 2012 at 2:26 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> H.J.,
>>> Question about this patch.
>>> Will it work if we simply replace
>>>    #if __WORDSIZE == 64
>>> with
>>>   #ifdef x86_64
>>> ?
>>>
>>> Today, x86_64 is the only 64-bit architecture supported by asan
>>> run-time on linux anyway.
>>>
>>>
>>
>> That works for me.
>>
>
> It should be
>
> #ifdef __x86_64__

Sure. Done, LLVM r167883.


>
> not
>
> #ifdef x86_64
>
> --
> H.J.

Patch

diff --git a/libsanitizer/sanitizer_common/sanitizer_linux.cc b/libsanitizer/sanitizer_common/sanitizer_linux.cc
index ab6c5a4..5d29018 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_linux.cc
@@ -34,7 +34,7 @@  namespace __sanitizer {
 // --------------- sanitizer_libc.h
 void *internal_mmap(void *addr, uptr length, int prot, int flags,
                     int fd, u64 offset) {
-#if __WORDSIZE == 64
+#if __WORDSIZE == 64 || defined __x86_64__
   return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset);
 #else
   return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
@@ -67,7 +67,7 @@  uptr internal_write(fd_t fd, const void *buf, uptr count) {
 }
 
 uptr internal_filesize(fd_t fd) {
-#if __WORDSIZE == 64
+#if __WORDSIZE == 64 || defined __x86_64__
   struct stat st;
   if (syscall(__NR_fstat, fd, &st))
     return -1;