Patchwork Enable building of libsanitizer on sparc linux again.

login
register
mail settings
Submitter Konstantin Serebryany
Date Nov. 18, 2012, 3:42 a.m.
Message ID <CAGQ9bdyvDRoRaN1j3JVvupUke2hqxBx7ipoy+eBQ+XfQKwoxQg@mail.gmail.com>
Download mbox | patch
Permalink /patch/199892/
State New
Headers show

Comments

Konstantin Serebryany - Nov. 18, 2012, 3:42 a.m.
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:













> 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.
Konstantin Serebryany - Nov. 18, 2012, 4:07 a.m.
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.
Konstantin Serebryany - Nov. 19, 2012, 8:12 a.m.
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.

Patch

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;