On Sun, Nov 18, 2012 at 7:34 AM, Andrew Pinski <pins...@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 7:17 PM, Konstantin Serebryany
> <konstantin.s.serebry...@gmail.com> wrote:
>> On Sat, Nov 17, 2012 at 7:10 PM, David Miller <da...@davemloft.net> wrote:
>>> From: Konstantin Serebryany <konstantin.s.serebry...@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.
