On Tue, Dec 03, 2013 at 07:18:14PM +0400, 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?
Of course. First of all all users trying to compile gcc just to find out
it won't build out of the box. Also users that were using asan just fine
on older platforms in gcc 4.8 and now they'll find out that the support
has been dropped.
> Maintaining asan&co on older platforms has a cost, and we are not
> willing to pay it;
Well, but then you just get approximately same cost if not higher in
maintaining configure bits for checking all the silent assumptions the code
makes and disabling libsanitizer in that case.
As you can see in the patches I've posted, most of the issues are in the
headers which you should be able to test the way I've posted yesterday, just
unpack a couple of .../usr/include trees from various distros. The only
ones not found by that are the .cfi_* stuff, you can test it by building
with -fno-dwarf2-cfi-asm and see whether it still builds, and
the tls size issue, which needs some solution in any case, because it is
just totally broken. Really ask the glibc folks for an API or talk to them
how to query it using libthread_db.so.1, struct pthread's size changes
frequently.
> > 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.
Why do you think it would be broken beyond repair?
> > 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.
Sure, just invent a macro for it and use it everywhere, for GCC that macro
can be defined based on whether __GCC_HAVE_DWARF2_CFI_ASM is defined, for
llvm/clang configure or whatever else.
> > #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.
> :(
If you never ask for it, it will never be done, it is that simple.
Anyway, does the code rely on exactly the right size of struct pthread, or
is a conservative minimum fine? Perhaps it is just a matter of adding
another case, if you tested say glibc 2.11, just don't do anything at all
for older glibcs (i.e. the same thing as for non-i?86/x86_64) - tls size 0.
Jakub
