| Message ID | CAGQ9bdyvDRoRaN1j3JVvupUke2hqxBx7ipoy+eBQ+XfQKwoxQg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
As for the libsanitizer update process, I suggest to move the discussion to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376 On Sun, Nov 18, 2012 at 7:42 AM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote: > On Sun, Nov 18, 2012 at 7:34 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Sat, Nov 17, 2012 at 7:17 PM, Konstantin Serebryany >> <konstantin.s.serebryany@gmail.com> wrote: >>> On Sat, Nov 17, 2012 at 7:10 PM, David Miller <davem@davemloft.net> wrote: >>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com> >>>> Date: Sat, 17 Nov 2012 19:01:56 -0800 >>>> >>>>> I am open to suggestions on how to avoid forking the two versions. >>>>> If we fork, the original asan team will not be able to cope with two >>>>> repositories. >>>> >>>> The maintainer of the sanitizer's job is to do the merging and resolve >>>> the conflicts between the two trees. This is how every other similar >>>> situation is handled. >>> >>> I am new to the gcc community and may not know all the rules. >>> But your nice words (lunacy, garbage, etc) are not helping us. >>> >>> As for the particular problem, I did not even see a patch (did I miss >>> it? Sorry, I am just back from a long trip) >>> I'd prefer to mention the ARCHs explicitly where possible, i.e. >>> #if defined(__x86_64__) || definde (__sparc64__) >>> instead of >>> #if __WORDSIZE == 64 || ... >> >> How about splitting this into a different config directory right now. > > Hm? > I don't think this is worth it, also we want the code to work for all > supported platforms in the LLVM tree too. > > My proposed patch is this: > > Index: sanitizer_linux.cc > =================================================================== > --- sanitizer_linux.cc (revision 168278) > +++ sanitizer_linux.cc (working copy) > @@ -31,12 +31,22 @@ > #include <unistd.h> > #include <errno.h> > > +// Are we using 32-bit or 64-bit syscalls? > +// We need to list the 64-bit architecures explicitly because for x32 > +// (which defines __x86_64__) we have __WORDSIZE == 32, > +// but we still need to use 64-bit syscalls. > +#if defined(__x86_64__) || defined(__powerpc64__) || defined(__sparc64__) > +# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1 > +#else > +# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1 > +#endif > + > namespace __sanitizer { > > // --------------- sanitizer_libc.h > void *internal_mmap(void *addr, uptr length, int prot, int flags, > int fd, u64 offset) { > -#if defined __x86_64__ > +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS > return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset); > #else > return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset); > @@ -69,7 +79,7 @@ > } > > uptr internal_filesize(fd_t fd) { > -#if defined __x86_64__ > +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS > struct stat st; > if (syscall(__NR_fstat, fd, &st)) > return -1; > @@ -95,7 +105,7 @@ > > // ----------------- sanitizer_common.h > bool FileExists(const char *filename) { > -#if defined __x86_64__ > +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS > struct stat st; > if (syscall(__NR_stat, filename, &st)) > return false; > > > > > > > > > > > > >> Maybe I will do this later today. This is what was needed when it was >> merged into GCC rather than all of these #ifdef all over the code. >> >> Look at how either libgomp or even glibc handles cases like this. >> They have include directories which is based on the target and maybe >> even a common directory which each target can over ride it (glibc is >> the best at doing this). >> >> The whole double review process is hard for the target maintainers of >> GCC to work really. Target maintainers in GCC is not normally like an >> extra review step as it does slow down the whole process of getting a >> target patch reviewed. >> >> >> Thanks, >> Andrew Pinski >> >>> >>> --kcc >>> >>>> >>>> What's happening here, frankly, is garbage. >>>> >>>> The current situation is unacceptable and HJ's fix should go into the >>>> GCC tree right now. >>>> >>>> The current situation is preventing people from getting work done, and >>>> unnecessarily consuming developer resources.
Upstream change http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?r1=168301&r2=168300&pathrev=168301 hopefully fixes the SPARC build. We need to resolve http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376 before we can automatically pull the fresh version into gcc. In the meantime, feel free to apply the exact same patch manually. --kcc On Sun, Nov 18, 2012 at 8:07 AM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote: > As for the libsanitizer update process, I suggest to move the > discussion to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376 > > On Sun, Nov 18, 2012 at 7:42 AM, Konstantin Serebryany > <konstantin.s.serebryany@gmail.com> wrote: >> On Sun, Nov 18, 2012 at 7:34 AM, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Sat, Nov 17, 2012 at 7:17 PM, Konstantin Serebryany >>> <konstantin.s.serebryany@gmail.com> wrote: >>>> On Sat, Nov 17, 2012 at 7:10 PM, David Miller <davem@davemloft.net> wrote: >>>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com> >>>>> Date: Sat, 17 Nov 2012 19:01:56 -0800 >>>>> >>>>>> I am open to suggestions on how to avoid forking the two versions. >>>>>> If we fork, the original asan team will not be able to cope with two >>>>>> repositories. >>>>> >>>>> The maintainer of the sanitizer's job is to do the merging and resolve >>>>> the conflicts between the two trees. This is how every other similar >>>>> situation is handled. >>>> >>>> I am new to the gcc community and may not know all the rules. >>>> But your nice words (lunacy, garbage, etc) are not helping us. >>>> >>>> As for the particular problem, I did not even see a patch (did I miss >>>> it? Sorry, I am just back from a long trip) >>>> I'd prefer to mention the ARCHs explicitly where possible, i.e. >>>> #if defined(__x86_64__) || definde (__sparc64__) >>>> instead of >>>> #if __WORDSIZE == 64 || ... >>> >>> How about splitting this into a different config directory right now. >> >> Hm? >> I don't think this is worth it, also we want the code to work for all >> supported platforms in the LLVM tree too. >> >> My proposed patch is this: >> >> Index: sanitizer_linux.cc >> =================================================================== >> --- sanitizer_linux.cc (revision 168278) >> +++ sanitizer_linux.cc (working copy) >> @@ -31,12 +31,22 @@ >> #include <unistd.h> >> #include <errno.h> >> >> +// Are we using 32-bit or 64-bit syscalls? >> +// We need to list the 64-bit architecures explicitly because for x32 >> +// (which defines __x86_64__) we have __WORDSIZE == 32, >> +// but we still need to use 64-bit syscalls. >> +#if defined(__x86_64__) || defined(__powerpc64__) || defined(__sparc64__) >> +# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1 >> +#else >> +# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1 >> +#endif >> + >> namespace __sanitizer { >> >> // --------------- sanitizer_libc.h >> void *internal_mmap(void *addr, uptr length, int prot, int flags, >> int fd, u64 offset) { >> -#if defined __x86_64__ >> +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS >> return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset); >> #else >> return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset); >> @@ -69,7 +79,7 @@ >> } >> >> uptr internal_filesize(fd_t fd) { >> -#if defined __x86_64__ >> +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS >> struct stat st; >> if (syscall(__NR_fstat, fd, &st)) >> return -1; >> @@ -95,7 +105,7 @@ >> >> // ----------------- sanitizer_common.h >> bool FileExists(const char *filename) { >> -#if defined __x86_64__ >> +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS >> struct stat st; >> if (syscall(__NR_stat, filename, &st)) >> return false; >> >> >> >> >> >> >> >> >> >> >> >> >>> Maybe I will do this later today. This is what was needed when it was >>> merged into GCC rather than all of these #ifdef all over the code. >>> >>> Look at how either libgomp or even glibc handles cases like this. >>> They have include directories which is based on the target and maybe >>> even a common directory which each target can over ride it (glibc is >>> the best at doing this). >>> >>> The whole double review process is hard for the target maintainers of >>> GCC to work really. Target maintainers in GCC is not normally like an >>> extra review step as it does slow down the whole process of getting a >>> target patch reviewed. >>> >>> >>> Thanks, >>> Andrew Pinski >>> >>>> >>>> --kcc >>>> >>>>> >>>>> What's happening here, frankly, is garbage. >>>>> >>>>> The current situation is unacceptable and HJ's fix should go into the >>>>> GCC tree right now. >>>>> >>>>> The current situation is preventing people from getting work done, and >>>>> unnecessarily consuming developer resources.
Index: sanitizer_linux.cc =================================================================== --- sanitizer_linux.cc (revision 168278) +++ sanitizer_linux.cc (working copy) @@ -31,12 +31,22 @@ #include <unistd.h> #include <errno.h> +// Are we using 32-bit or 64-bit syscalls? +// We need to list the 64-bit architecures explicitly because for x32 +// (which defines __x86_64__) we have __WORDSIZE == 32, +// but we still need to use 64-bit syscalls. +#if defined(__x86_64__) || defined(__powerpc64__) || defined(__sparc64__) +# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1 +#else +# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1 +#endif + namespace __sanitizer { // --------------- sanitizer_libc.h void *internal_mmap(void *addr, uptr length, int prot, int flags, int fd, u64 offset) { -#if defined __x86_64__ +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS return (void *)syscall(__NR_mmap, addr, length, prot, flags, fd, offset); #else return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset); @@ -69,7 +79,7 @@ } uptr internal_filesize(fd_t fd) { -#if defined __x86_64__ +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS struct stat st; if (syscall(__NR_fstat, fd, &st)) return -1; @@ -95,7 +105,7 @@ // ----------------- sanitizer_common.h bool FileExists(const char *filename) { -#if defined __x86_64__ +#if SANITIZER_LINUX_USES_64BIT_SYSCALLS struct stat st; if (syscall(__NR_stat, filename, &st)) return false;