| Message ID | 20121112233606.GA6375@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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?
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.
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.
> 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.
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.
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
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
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.
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
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
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.
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
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
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.
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
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.
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.
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.
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.
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
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.
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;