| Message ID | 20131203114959.GW892@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
On 12/03/13 08:47, Jakub Jelinek wrote: > 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. Right. We certainly care. Those old platforms are still in widespread use (for better or worse). > >> 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. Right. Fixing this problem and fixing it correctly will reduce the long term pain for both projects. >> >> 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. glibc pushes out a release every six months which then gets put into the next Fedora release which usually follows a few months later. We're talking about a worst case lag time of roughly 9 months, often much less. It's probably similar for the SuSE community releases. I think everyone who has had a bad experience working with glibc's team in the past should try to put it behind them. The new team running glibc is much more reasonable to work with -- the biggest problem now is rebooting the project after years of mis-management. The progress they've made towards that over the last year has been tremendous. jeff
On Tue, Dec 03, 2013 at 07:18:14PM +0400, Konstantin Serebryany wrote: > > ==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 BTW, just to fill in some of the missing data from a couple of glibcs: glibc 2.3.6 FIRST_32_SECOND_64(1104, 1696) glibc 2.4 FIRST_32_SECOND_64(1120, 1728) glibc 2.5 FIRST_32_SECOND_64(1136, 1728) glibc 2.6, 2.7, 2.8, 2.9 FIRST_32_SECOND_64(1136, 1712) glibc 2.10.1 FIRST_32_SECOND_64(1168, 1776) glibc 2.11.1, 2.12 FIRST_32_SECOND_64(1168, 2288) glibc 2.13, 2.14.1, 2.15, 2.17 FIRST_32_SECOND_64(1216, 2304) script to extract the data was: mkdir /tmp/aa; cd /tmp/aa; for i in /tmp/glibc-2.*; do echo $i; rm -rf /tmp/aa/*; rpm2cpio $i | cpio -id; readelf -Ws lib*/libpthread-2.*.so | grep '_thread_db_sizeof_pthread$' | awk '{print $2}'; j=`readelf -Ws lib*/libpthread-2.*.so | grep '_thread_db_sizeof_pthread$' | awk '{print $2}' | sed 's/[48c]$/0/;s/^00*//'`; objdump -s -j .rodata lib*/libpthread-2.*.so | grep $j; done So, as the data shows the numbers aren't even always monotonically increasing. Jakub
>>> >>> 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. > > glibc pushes out a release every six months which then gets put into the > next Fedora release which usually follows a few months later. We're talking > about a worst case lag time of roughly 9 months, often much less. It's > probably similar for the SuSE community releases. > > I think everyone who has had a bad experience working with glibc's team in > the past should try to put it behind them. The new team running glibc is > much more reasonable to work with -- the biggest problem now is rebooting > the project after years of mis-management. The progress they've made > towards that over the last year has been tremendous. > Submitted a feature request against glibc https://sourceware.org/bugzilla/show_bug.cgi?id=16291 Please comment there if you think I did not provide enough information. --kcc
On Tue, Dec 3, 2013 at 5:42 PM, Jeff Law <law@redhat.com> wrote: > On 12/03/13 08:47, Jakub Jelinek wrote: >> >> 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. > > Right. We certainly care. Those old platforms are still in widespread use > (for better or worse). So, would you be able to help us upstream? We need a) patches that we can review and apply to the llvm repository (w/o breaking the modern systems, of course) b) a buildbot that would run 24/7 catching regressions. If we reach a green state for a platform X and have a buildbot for it, keeping it green will require relatively small effort. Every time we break it we will notice it in minutes and fix quickly while we still have the same context fresh. Fixing old systems once in few months during merge to gcc is costly because failures accumulate. --kcc > > >> >>> 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. > > Right. Fixing this problem and fixing it correctly will reduce the long > term pain for both projects. > > > >>> >>> 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. > > glibc pushes out a release every six months which then gets put into the > next Fedora release which usually follows a few months later. We're talking > about a worst case lag time of roughly 9 months, often much less. It's > probably similar for the SuSE community releases. > > I think everyone who has had a bad experience working with glibc's team in > the past should try to put it behind them. The new team running glibc is > much more reasonable to work with -- the biggest problem now is rebooting > the project after years of mis-management. The progress they've made > towards that over the last year has been tremendous. > > jeff > >
On 12/03/13 22:08, Konstantin Serebryany wrote: > We need > a) patches that we can review and apply to the llvm repository (w/o > breaking the modern systems, of course) > b) a buildbot that would run 24/7 catching regressions. > > If we reach a green state for a platform X and have a buildbot for it, > keeping it green will require relatively small effort. > Every time we break it we will notice it in minutes and fix quickly > while we still have the same context fresh. > Fixing old systems once in few months during merge to gcc is costly > because failures accumulate. I'm well overbooked already. However, if you have x86/x86_64 systems in your build farm that can be virtualized, I can help set up a suitable VM. CentOS 5.x is old enough to trigger lots of interesting problems, but is still in widespread use. Jeff
On Thu, Dec 5, 2013 at 1:05 AM, Jeff Law <law@redhat.com> wrote: > On 12/03/13 22:08, Konstantin Serebryany wrote: >> >> We need >> a) patches that we can review and apply to the llvm repository (w/o >> breaking the modern systems, of course) >> b) a buildbot that would run 24/7 catching regressions. >> >> If we reach a green state for a platform X and have a buildbot for it, >> keeping it green will require relatively small effort. >> Every time we break it we will notice it in minutes and fix quickly >> while we still have the same context fresh. >> Fixing old systems once in few months during merge to gcc is costly >> because failures accumulate. > > I'm well overbooked already. However, if you have x86/x86_64 systems in > your build farm that can be virtualized, Unfortunately we don't. In case anyone wants to provide their build machines and maintain a bot on them here are the instructions (Evgeniy and Alexey in CC may be able to help) --kcc > I can help set up a suitable VM. > CentOS 5.x is old enough to trigger lots of interesting problems, but is > still in widespread use. > > Jeff > >
On Thu, Dec 5, 2013 at 4:22 PM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote: > On Thu, Dec 5, 2013 at 1:05 AM, Jeff Law <law@redhat.com> wrote: >> On 12/03/13 22:08, Konstantin Serebryany wrote: >>> >>> We need >>> a) patches that we can review and apply to the llvm repository (w/o >>> breaking the modern systems, of course) >>> b) a buildbot that would run 24/7 catching regressions. >>> >>> If we reach a green state for a platform X and have a buildbot for it, >>> keeping it green will require relatively small effort. >>> Every time we break it we will notice it in minutes and fix quickly >>> while we still have the same context fresh. >>> Fixing old systems once in few months during merge to gcc is costly >>> because failures accumulate. >> >> I'm well overbooked already. However, if you have x86/x86_64 systems in >> your build farm that can be virtualized, > > Unfortunately we don't. > In case anyone wants to provide their build machines and maintain a > bot on them here are the instructions Actual link: http://llvm.org/docs/HowToAddABuilder.html > (Evgeniy and Alexey in CC may be able to help) > > --kcc > >> I can help set up a suitable VM. >> CentOS 5.x is old enough to trigger lots of interesting problems, but is >> still in widespread use. >> >> Jeff >> >>
--- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h.jj3 2013-12-03 03:33:20.000000000 -0500 +++ libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h 2013-12-03 05:26:41.864437661 -0500 @@ -140,23 +140,32 @@ namespace __sanitizer { int gid; int cuid; int cgid; -#ifdef __powerpc64__ +#ifdef __powerpc__ unsigned mode; unsigned __seq; + u64 __unused1; + u64 __unused2; #else unsigned short mode; unsigned short __pad1; unsigned short __seq; unsigned short __pad2; +#if defined(__x86_64__) && !defined(_LP64) + u64 __unused1; + u64 __unused2; +#else + unsigned long __unused1; + unsigned long __unused2; +#endif #endif - uptr __unused1; - uptr __unused2; }; struct __sanitizer_shmid_ds { __sanitizer_ipc_perm shm_perm; #ifndef __powerpc__ uptr shm_segsz; + #elif !defined(__powerpc64__) + uptr __unused0; #endif uptr shm_atime; #ifndef _LP64 @@ -288,17 +297,20 @@ namespace __sanitizer { typedef long __sanitizer_clock_t; #if SANITIZER_LINUX -#if defined(_LP64) || defined(__x86_64__) +#if defined(_LP64) || defined(__x86_64__) || defined(__powerpc__) typedef unsigned __sanitizer___kernel_uid_t; typedef unsigned __sanitizer___kernel_gid_t; - typedef long long __sanitizer___kernel_off_t; #else typedef unsigned short __sanitizer___kernel_uid_t; typedef unsigned short __sanitizer___kernel_gid_t; +#endif +#if defined(__x86_64__) && !defined(_LP64) + typedef long long __sanitizer___kernel_off_t; +#else typedef long __sanitizer___kernel_off_t; #endif -#if defined(__powerpc64__) +#if defined(__powerpc__) typedef unsigned int __sanitizer___kernel_old_uid_t; typedef unsigned int __sanitizer___kernel_old_gid_t; #else