Patchwork Enable building of libsanitizer on sparc linux again.

login
register
mail settings
Submitter Dodji Seketeli
Date Nov. 15, 2012, 10:56 a.m.
Message ID <87vcd7w2vr.fsf@redhat.com>
Download mbox | patch
Permalink /patch/199248/
State New
Headers show

Comments

Dodji Seketeli - Nov. 15, 2012, 10:56 a.m.
David Miller <davem@davemloft.net> wrote

> From: Dodji Seketeli <dodji@redhat.com>
> Date: Wed, 14 Nov 2012 14:26:40 +0100
> 
> > I guess we could do that.  That would build libsanitizer, but asan will
> > still not be available on sparc if the asan_shadow_offset() target hook
> > is not provided.  Is that OK to you?
> 
> Yes.

So, here is the (IMO obvious) patch to enable libsanitizer's build on
sparc linux, even if asan is not supported on that platform yet.

OK for trunk?

libsanitizer/ChangeLog:

	* configure.tgt: Enable sparc linux.
---
 libsanitizer/configure.tgt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Diego Novillo - Nov. 15, 2012, 3:31 p.m.
On Thu, Nov 15, 2012 at 5:56 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> David Miller <davem@davemloft.net> wrote
>
>> From: Dodji Seketeli <dodji@redhat.com>
>> Date: Wed, 14 Nov 2012 14:26:40 +0100
>>
>> > I guess we could do that.  That would build libsanitizer, but asan will
>> > still not be available on sparc if the asan_shadow_offset() target hook
>> > is not provided.  Is that OK to you?
>>
>> Yes.
>
> So, here is the (IMO obvious) patch to enable libsanitizer's build on
> sparc linux, even if asan is not supported on that platform yet.
>
> OK for trunk?
>
> libsanitizer/ChangeLog:
>
>         * configure.tgt: Enable sparc linux.

If this does not break the sparc build, it's OK.


Diego.
David Miller - Nov. 15, 2012, 10:58 p.m.
From: Dodji Seketeli <dodji@redhat.com>
Date: Thu, 15 Nov 2012 11:56:40 +0100

> David Miller <davem@davemloft.net> wrote
> 
>> From: Dodji Seketeli <dodji@redhat.com>
>> Date: Wed, 14 Nov 2012 14:26:40 +0100
>> 
>> > I guess we could do that.  That would build libsanitizer, but asan will
>> > still not be available on sparc if the asan_shadow_offset() target hook
>> > is not provided.  Is that OK to you?
>> 
>> Yes.
> 
> So, here is the (IMO obvious) patch to enable libsanitizer's build on
> sparc linux, even if asan is not supported on that platform yet.
> 
> OK for trunk?

I'm fine with it.
Dodji Seketeli - Nov. 16, 2012, 8:11 a.m.
David Miller <davem@davemloft.net> writes:

> From: Dodji Seketeli <dodji@redhat.com>
> Date: Thu, 15 Nov 2012 11:56:40 +0100
>
>> David Miller <davem@davemloft.net> wrote
>> 
>>> From: Dodji Seketeli <dodji@redhat.com>
>>> Date: Wed, 14 Nov 2012 14:26:40 +0100
>>> 
>>> > I guess we could do that.  That would build libsanitizer, but asan will
>>> > still not be available on sparc if the asan_shadow_offset() target hook
>>> > is not provided.  Is that OK to you?
>>> 
>>> Yes.
>> 
>> So, here is the (IMO obvious) patch to enable libsanitizer's build on
>> sparc linux, even if asan is not supported on that platform yet.
>> 
>> OK for trunk?
>
> I'm fine with it.

Thanks.  Committed to trunk, revision r193552.
Eric Botcazou - Nov. 17, 2012, 11:18 p.m.
> So, here is the (IMO obvious) patch to enable libsanitizer's build on
> sparc linux, even if asan is not supported on that platform yet.
> 
> OK for trunk?
> 
> libsanitizer/ChangeLog:
> 
> 	* configure.tgt: Enable sparc linux.

libtool: compile:  /home/ebotcazou/build/./gcc/g++ -
B/home/ebotcazou/build/./gcc/ -nostdinc++ -nostdinc++ -
I/home/ebotcazou/build/sparc64-linux-gnu/64/libstdc++-v3/include/sparc64-
linux-gnu -I/home/ebotcazou/build/sparc64-linux-gnu/64/libstdc++-v3/include -
I/home/ebotcazou/src/libstdc++-v3/libsupc++ -I/home/ebotcazou/src/libstdc++-
v3/include/backward -I/home/ebotcazou/src/libstdc++-v3/testsuite/util -
L/home/ebotcazou/build/sparc64-linux-gnu/64/libstdc++-v3/src -
L/home/ebotcazou/build/sparc64-linux-gnu/64/libstdc++-v3/src/.libs -
B/home/ebotcazou/install/sparc64-linux-gnu/bin/ -
B/home/ebotcazou/install/sparc64-linux-gnu/lib/ -isystem 
/home/ebotcazou/install/sparc64-linux-gnu/include -isystem 
/home/ebotcazou/install/sparc64-linux-gnu/sys-include -m64 -D_GNU_SOURCE -
D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-I. -I/home/ebotcazou/src/libsanitizer/sanitizer_common -I 
/home/ebotcazou/src/libsanitizer/include -Wall -W -Wno-unused-parameter -
Wwrite-strings -pedantic -Wno-long-long -fPIC -fno-builtin -fno-exceptions -
fomit-frame-pointer -funwind-tables -fvisibility=hidden -Wno-variadic-macros -
Wno-c99-extensions -g -O2 -D_GNU_SOURCE -m64 -MT sanitizer_linux.lo -MD -MP -
MF .deps/sanitizer_linux.Tpo -c 
/home/ebotcazou/src/libsanitizer/sanitizer_common/sanitizer_linux.cc  -fPIC -
DPIC -o .libs/sanitizer_linux.o
/home/ebotcazou/src/libsanitizer/sanitizer_common/sanitizer_linux.cc: In 
function 'void* __sanitizer::internal_mmap(void*, __sanitizer::uptr, int, int, 
int, __sanitizer::u64)':
/home/ebotcazou/src/libsanitizer/sanitizer_common/sanitizer_linux.cc:40:26: 
error: '__NR_mmap2' was not declared in this scope
   return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
                          ^
/home/ebotcazou/src/libsanitizer/sanitizer_common/sanitizer_linux.cc:42:1: 
warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
At global scope:
cc1plus: warning: unrecognized command line option "-Wno-c99-extensions" 
[enabled by default]
make[5]: *** [sanitizer_linux.lo] Error 1
David Miller - Nov. 18, 2012, 12:04 a.m.
From: Eric Botcazou <ebotcazou@adacore.com>
Date: Sun, 18 Nov 2012 00:18:15 +0100

> error: '__NR_mmap2' was not declared in this scope
>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);

The people making libsanitizer changes are really going to have to
stop making i386 specific changes to these generic files.

Specifically, in this case, they are checking for whether mmap2 is
available using __x86_64__ cpp tests.  A more appropriate test is
necessary.

Oh nevermind, H.J. Liu added this build regression, I should have
known.

H.J., either fix or revert this code back to using __WORDSIZE if you
cannot come up with an appropriate test.
H.J. Lu - Nov. 18, 2012, 12:06 a.m.
On Sat, Nov 17, 2012 at 4:04 PM, David Miller <davem@davemloft.net> wrote:
> From: Eric Botcazou <ebotcazou@adacore.com>
> Date: Sun, 18 Nov 2012 00:18:15 +0100
>
>> error: '__NR_mmap2' was not declared in this scope
>>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
>
> The people making libsanitizer changes are really going to have to
> stop making i386 specific changes to these generic files.
>
> Specifically, in this case, they are checking for whether mmap2 is
> available using __x86_64__ cpp tests.  A more appropriate test is
> necessary.
>
> Oh nevermind, H.J. Liu added this build regression, I should have
> known.
>
> H.J., either fix or revert this code back to using __WORDSIZE if you
> cannot come up with an appropriate test.

Please follow:

http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html
David Miller - Nov. 18, 2012, 12:14 a.m.
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 17 Nov 2012 16:06:23 -0800

> On Sat, Nov 17, 2012 at 4:04 PM, David Miller <davem@davemloft.net> wrote:
>> From: Eric Botcazou <ebotcazou@adacore.com>
>> Date: Sun, 18 Nov 2012 00:18:15 +0100
>>
>>> error: '__NR_mmap2' was not declared in this scope
>>>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
>>
>> The people making libsanitizer changes are really going to have to
>> stop making i386 specific changes to these generic files.
>>
>> Specifically, in this case, they are checking for whether mmap2 is
>> available using __x86_64__ cpp tests.  A more appropriate test is
>> necessary.
>>
>> Oh nevermind, H.J. Liu added this build regression, I should have
>> known.
>>
>> H.J., either fix or revert this code back to using __WORDSIZE if you
>> cannot come up with an appropriate test.
> 
> Please follow:
> 
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html

The whole way this libsanitizer merge is being handled is
beyond unreasonable.

A build fix being held up for 4 days just proves that requiring things
get merged into LLVM first is completely the wrong way for this stuff
to work.

The people who merged in this library should be responsible for
keeping changes in sync, not the individual developers who fix bugs
and make improvements to this code on the gcc side.

It's lunacy that this build problem is in the tree for 4 days because
of this.
Konstantin Serebryany - Nov. 18, 2012, 3:01 a.m.
On Sat, Nov 17, 2012 at 4:14 PM, David Miller <davem@davemloft.net> wrote:
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Sat, 17 Nov 2012 16:06:23 -0800
>
>> On Sat, Nov 17, 2012 at 4:04 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Eric Botcazou <ebotcazou@adacore.com>
>>> Date: Sun, 18 Nov 2012 00:18:15 +0100
>>>
>>>> error: '__NR_mmap2' was not declared in this scope
>>>>    return (void *)syscall(__NR_mmap2, addr, length, prot, flags, fd, offset);
>>>
>>> The people making libsanitizer changes are really going to have to
>>> stop making i386 specific changes to these generic files.
>>>
>>> Specifically, in this case, they are checking for whether mmap2 is
>>> available using __x86_64__ cpp tests.  A more appropriate test is
>>> necessary.
>>>
>>> Oh nevermind, H.J. Liu added this build regression, I should have
>>> known.
>>>
>>> H.J., either fix or revert this code back to using __WORDSIZE if you
>>> cannot come up with an appropriate test.
>>
>> Please follow:
>>
>> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00951.html
>
> The whole way this libsanitizer merge is being handled is
> beyond unreasonable.
>
> A build fix being held up for 4 days just proves that requiring things
> get merged into LLVM first is completely the wrong way for this stuff
> to work.

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.

--kcc

>
> The people who merged in this library should be responsible for
> keeping changes in sync, not the individual developers who fix bugs
> and make improvements to this code on the gcc side.
>
> It's lunacy that this build problem is in the tree for 4 days because
> of this.
David Miller - Nov. 18, 2012, 3:10 a.m.
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.

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, 3:17 a.m.
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 || ...

--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.
Diego Novillo - Nov. 18, 2012, 3:26 a.m.
On Sat, Nov 17, 2012 at 10: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.
>
> What's happening here, frankly, is garbage.

Calm down, David.  I understand this is frustrating, but reacting in
this manner is not helpful to anyone.  We have some new maintainers
that are trying to understand how the system works.  Insulting and
berating them will only encourage them to pack up and leave.

There is no need to do any forking.

Kostya, would it be acceptable if fixes that go in the gcc tree get
then propagated to the LLVM tree?  The two trees don't need to be kept
in sync at every commit.  Patches to the GCC tree will be in your
inbox or submitted to gcc-patches.


Diego.
David Miller - Nov. 18, 2012, 3:28 a.m.
From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Sat, 17 Nov 2012 19:17:17 -0800

> I'd prefer to mention the ARCHs explicitly where possible, i.e.
>   #if defined(__x86_64__) || definde (__sparc64__)
> instead of
>    #if __WORDSIZE == 64 || ...

You really do need to check __x86_64__ as well the word size, in order
to distinguish x32 from traditional x86-64.

So no, it is not possible to just use an ARCH check all by itself
to handle this case.

Therefore, HJ's check is the correct check, and mirrors similar
existing checks elsewhere in the tree (f.e. libffi).
David Miller - Nov. 18, 2012, 3:32 a.m.
From: Diego Novillo <dnovillo@google.com>
Date: Sat, 17 Nov 2012 22:26:24 -0500

> We have some new maintainers that are trying to understand how the
> system works.

Wouldn't we have someone become at least roughly familiar with these
kinds of things before we allow them to commit such an invasive set of
changes which have a non-trivial effect on pretty much everyone?

You cannot get around the fact that this was all handled poorly.
Konstantin Serebryany - Nov. 18, 2012, 3:32 a.m.
On Sat, Nov 17, 2012 at 7:26 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Sat, Nov 17, 2012 at 10: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.
>>
>> What's happening here, frankly, is garbage.
>
> Calm down, David.  I understand this is frustrating, but reacting in
> this manner is not helpful to anyone.  We have some new maintainers
> that are trying to understand how the system works.  Insulting and
> berating them will only encourage them to pack up and leave.
>
> There is no need to do any forking.
>
> Kostya, would it be acceptable if fixes that go in the gcc tree get
> then propagated to the LLVM tree?

It may be acceptable for trivial patches, but we still want to review them.
Once we review patch, it is easier for us to put it into LLVM first
and then to gcc.
Which reminds me, that libsanitizer/README.gcc is not helping in this
process yet...

>  The two trees don't need to be kept
> in sync at every commit.  Patches to the GCC tree will be in your
> inbox or submitted to gcc-patches.
>
>
> Diego.
Andrew Pinski - Nov. 18, 2012, 3:34 a.m.
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.
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.
David Miller - Nov. 18, 2012, 3:38 a.m.
From: Andrew Pinski <pinskia@gmail.com>
Date: Sat, 17 Nov 2012 19:34:34 -0800

> (glibc is the best at doing this).

It also uses "make" in pretty much the most inefficient way possible,
by causing it to consider 10s of thousands of prefix rules for every
rule target.

GLIBC has the same ifdef check that is being suggested here in various
places.

Patch

diff --git a/libsanitizer/configure.tgt b/libsanitizer/configure.tgt
index ca7ac1f..988312e 100644
--- a/libsanitizer/configure.tgt
+++ b/libsanitizer/configure.tgt
@@ -20,7 +20,7 @@ 
 
 # Filter out unsupported systems.
 case "${target}" in
-  x86_64-*-linux* | i?86-*-linux*)
+  x86_64-*-linux* | i?86-*-linux* | sparc*-*-linux*)
 	;;
   *)
 	UNSUPPORTED=1