This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: libsanitizer merge from upstream r196090


On Tue, Dec 3, 2013 at 3:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Dec 02, 2013 at 05:43:17PM +0100, Konstantin Serebryany wrote:
>> > No, so your patch doesn't regress anything. I can configure with
>> > --disable-libsanitizer to skip build of libsanitizer, although it
>> > would be nice to support RHEL5 derived long-term distributions.
>> Ok, so this does not gate the merge.
>
> Well, it regresses against 4.8, so it still is a P1 regression.

Does anyone care?
Maintaining asan&co on older platforms has a cost, and we are not
willing to pay it;
even reviewing patches like yours (still, thanks!) requires time.
The only long term strategy to support old systems is if someone works
closely with us in upstream
and we keep the code clean all the time.
If no one cares enough to do that, why should we?

I'll look at your second patch though.

>
> BTW, even the merge itself apparently regresses, powerpc* doesn't build any
> longer and it did before this merge.

The current llvm trunk builds on powerpc64 fine (just checked); we
build only the 64-bit variant.
I suppose that your patch fixes the 32-bit build.
It is broken beyond repair, I don't have any indication anyone ever used it.
Unless I hear otherwise I propose to disable 32-bit powerpc build.
64-bit variant is broken too (tests don't pass), but at least it builds.
If someone wants usable 32-bit powerpc they need to work with us upstream.

> The first attached patch fixes the build on powerpc* (tested on RHEL6/ppc*)
> and the second patch fixes the build on RHEL5/x86_64, beyond what I've
> posted earlier the patch attempts to handle the .cfi* stuff (tried looking
> if clang defines some usable macro, but couldn't find any, so no idea how
> you can find out if you are compiled with clang -fno-dwarf2-cfi-asm

Yes, we will nee to find out some other macro to hide .CFI and such.
Maybe just something like SANITIZER_ALLOW_CFI_ASM or similar.
Again, we need to keep the code clean all the time in upstream,
otherwise gcc merges get too expensive.

> (when tsan will probably not build at all), or with the -fdwarf2-cfi-asm
> default.  Probably either you need to convince llvm folks to add something,
> or add configure test, moved the linux/mroute* headers to the Linux specific
> file so that linux/in.h stuff doesn't clash with netinet/in.h etc. and
> finally hacks to avoid including linux/types.h at all costs, that header
> historically has always been terrible portability minefield, as it defines
> types that glibc headers define too.
>
> With these two patches on top of your patch, I get libsanitizer to build
> in multilib configurations on Fedora19/x86_64, RHEL6/ppc* and RHEL5/x86_64
> (in all cases both 32-bit and 64-bit builds, but not the third x32 multilib).
>
> Testsuite passes on Fedora19, on RHEL5/x86_64 tests that fail typically fail
> on
> ==2738==AddressSanitizer CHECK failed:
> ../../../../libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:260 "((*tls_addr + *tls_size)) <= ((*stk_addr + *stk_size))" (0x2af8df1bc240, 0x2af8df1bc000)
> which clearly is a bug in sanitizer_common,
>
> #if defined(__x86_64__) || defined(__i386__)
> // sizeof(struct thread) from glibc.
> // There has been a report of this being different on glibc 2.11 and 2.13. We
> // don't know when this change happened, so 2.14 is a conservative estimate.
> #if __GLIBC_PREREQ(2, 14)
> const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1216, 2304);
> #else
> const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1168, 2304);
> #endif



>
> uptr ThreadDescriptorSize() {
>   return kThreadDescriptorSize;
> }
> but also the InitTlsSize hack can't ever work reliably.
> If you need this info, ask glibc folks for a proper supported API.

This won't happen any time soon, right?
I'd like to ask glibc for many other things, not just this, but the latency
of the glibc changes propagating to users is too low, so we don't
bother (although we should)
E.g. we've been hit by the ugly
pthread_getattr_np/pthread_attr_getstack multiple times.
:(

> Hardcoding size of glibc private structure that glibc has changed multiple
> times (note, RHEL5 has glibc 2.5 and clearly the size doesn't match there
> even with the one you've computed on glibc 2.11) will most likely break any
> time glibc adds further fields in there, not to mention that the above
> means that if you e.g. build libsanitizer say on glibc 2.11 and run against
> glibc 2.14, it will also not work.  Plus calling _dl_get_tls_static_info,
> which is a GLIBC_PRIVATE symbol, is also going to break whenever glibc
> decides to change the ABI of that function.
> I believe libthread_db.so.1 has some APIs to query sizeof (struct pthread),
> but have never used those APIs so don't know how to query that.

I think one of us tried that with not success.

> My recommendation would be to just ifdef out this for now until you use
> some sane reliable and supportable API.  Otherwise, it may work if you are
> lucky and always build libsanitizer against the exact same version of glibc
> as you run it against, perhaps that is the only use model that llvm cares
> about, but for GCC that is definitely not acceptable.
>
>         Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]