diff mbox

libsanitizer merge from upstream r196090

Message ID CAGQ9bdw6cW6n1_cYm0V5ToXQ8t3zYfe4PBDd7Kb+29ke+ADELw@mail.gmail.com
State New
Headers show

Commit Message

Konstantin Serebryany Dec. 4, 2013, 1:28 p.m. UTC 
On Wed, Dec 4, 2013 at 5:02 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 04, 2013 at 04:49:22PM +0400, Konstantin Serebryany wrote:
>> I would start from kernel version and glibc version, this should cover
>> the majority of use cases.
>
> Well, for the kernel headers what we perhaps can do is just add
> libsanitizer/include/linux/ tree that will be maintained by GCC and will

if that works for you, no objections.

> contain (where needed) wrappers for kernel headers or their replacements
> to make sure things compile, if you don't care about it in the compiler-rt
> tree.  But for the ppc32 stuff, we can't avoid modifying sanitizer_common
> (the first patch I've posted recently, btw, I wonder if it works on sparc*,
> we'll need to wait for somebody to test it), or e.g. for the .cfi_* stuff
> (if you just apply the the .cfi_* related part of the patch I've posted
> with say the macros __GCC_HAVE_* replaced by SANITIZER_USE_CFI_DIRECTIVES
> or similar, I guess we can provide the right definition for that outside of
> the compiler-rt maintained files.

.cfi is used only in tsan sources now, and tsan is not supported
anywhere but x86_64
ppc32 never worked (last time I tried there were several different
issues so we disabled 32-bit build)
-- we should just disable it in GCC too. There is not value in
building code that does not run.

> Regarding the TLS size, can you e.g. just only do it for glibc 2.13 and
> later, rather than having an (even for glibc 2.11/2.12 incorrect) values for
> older glibcs?

That would work for me, although it may bring some surprises later.
If we incorrectly compute the tls boundaries, lsan my produce false
positives or false negatives.
Having kThreadDescriptorSize=0 means that we include the stack
descriptor in the lsan's root set and thus
may miss a leak (with rather low probability). I can live with this.

Like this (tested only on my box)?



<grumbling>
If I had a buildbot with "old" Fedora, I would simply submit the
change and see if it broke/fixed it.
</grumbling>

--kcc



>
>         Jakub

Comments

Jakub Jelinek Dec. 4, 2013, 1:44 p.m. UTC | #1 
On Wed, Dec 04, 2013 at 05:28:40PM +0400, Konstantin Serebryany wrote:
> > Well, for the kernel headers what we perhaps can do is just add
> > libsanitizer/include/linux/ tree that will be maintained by GCC and will
> 
> if that works for you, no objections.

I haven't tried to do that yet, so don't know how much work it will be,
but at least from the second patch posted recently it it might work fine, at
least for now.

> .cfi is used only in tsan sources now, and tsan is not supported
> anywhere but x86_64

But the .cfi_* issue is platform independent.  Whether the compiler
decides to emit them or not depends on how it was configured, on assembler
and on compiler flags.
I don't see how it can be a maintainance problem to just guard the few
(right now two) .cfi_* occurrences in the C++ sources, or using CFI_* macros
instead of .cfi_* directives directly in the assembly file.
Other projects (e.g. glibc) manage to do that for years without any trouble.

> ppc32 never worked (last time I tried there were several different
> issues so we disabled 32-bit build)
> -- we should just disable it in GCC too. There is not value in
> building code that does not run.

That doesn't mean it can't be made to work, and the patch I've posted is
at least an (IMHO correct) step towards that.  Note, I had just much bigger
problems on ppc64 with the addr2line symbolization because of the ppc64
opd/plt stuff, though supposedly that might go away once I patch
libsanitizer to use libbacktrace for symbolization.
There is no inherent reason why ppc32 wouldn't work and ppc64 would, after
all ppc64 with its weirdo function descriptor stuff is much harder to
support.

> > Regarding the TLS size, can you e.g. just only do it for glibc 2.13 and
> > later, rather than having an (even for glibc 2.11/2.12 incorrect) values for
> > older glibcs?
> 
> That would work for me, although it may bring some surprises later.
> If we incorrectly compute the tls boundaries, lsan my produce false
> positives or false negatives.

But is that solely for lsan and nothing else?  Because, the assertion
was failing in asan tests, without any asan options to request leak
checking.  And for non-i?86/x86_64 you ignore the tls boundaries too.

> Having kThreadDescriptorSize=0 means that we include the stack
> descriptor in the lsan's root set and thus
> may miss a leak (with rather low probability). I can live with this.
> 
> Like this (tested only on my box)?

> --- sanitizer_linux_libcdep.cc  (revision 196375)
> +++ sanitizer_linux_libcdep.cc  (working copy)
> @@ -207,12 +207,12 @@
> 
>  #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)
> +// This may change between glibc versions, we only support the versions we know
> +// avout (>= 2.13). For others we set kThreadDescriptorSize to 0.
> +#if __GLIBC_PREREQ(2, 13)
>  const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1216, 2304);
>  #else
> -const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1168, 2304);
> +const uptr kThreadDescriptorSize = 0;  // Unknown.

Depends on (as I've asked earlier) on if you need the exact precise
value or if say conservatively smaller value is fine.  Then you could
say for glibc >= 2.5 pick the minimum of the values I've gathered.

	Jakub
Konstantin Serebryany Dec. 4, 2013, 2:17 p.m. UTC | #2 
On Wed, Dec 4, 2013 at 5:44 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 04, 2013 at 05:28:40PM +0400, Konstantin Serebryany wrote:
>> > Well, for the kernel headers what we perhaps can do is just add
>> > libsanitizer/include/linux/ tree that will be maintained by GCC and will
>>
>> if that works for you, no objections.
>
> I haven't tried to do that yet, so don't know how much work it will be,
> but at least from the second patch posted recently it it might work fine, at
> least for now.
>
>> .cfi is used only in tsan sources now, and tsan is not supported
>> anywhere but x86_64
>
> But the .cfi_* issue is platform independent.  Whether the compiler
> decides to emit them or not depends on how it was configured, on assembler
> and on compiler flags.
> I don't see how it can be a maintainance problem to just guard the few
> (right now two) .cfi_* occurrences in the C++ sources, or using CFI_* macros
> instead of .cfi_* directives directly in the assembly file.
> Other projects (e.g. glibc) manage to do that for years without any trouble.

replied separately.

>
>> ppc32 never worked (last time I tried there were several different
>> issues so we disabled 32-bit build)
>> -- we should just disable it in GCC too. There is not value in
>> building code that does not run.
>
> That doesn't mean it can't be made to work, and the patch I've posted is
> at least an (IMHO correct) step towards that.

Sure it can. But all my previous grumbling about maintenance cost and
our inability to test changes, etc applies here.

>   Note, I had just much bigger
> problems on ppc64 with the addr2line symbolization because of the ppc64
> opd/plt stuff, though supposedly that might go away once I patch
> libsanitizer to use libbacktrace for symbolization.
> There is no inherent reason why ppc32 wouldn't work and ppc64 would, after
> all ppc64 with its weirdo function descriptor stuff is much harder to
> support.
>
>> > Regarding the TLS size, can you e.g. just only do it for glibc 2.13 and
>> > later, rather than having an (even for glibc 2.11/2.12 incorrect) values for
>> > older glibcs?
>>
>> That would work for me, although it may bring some surprises later.
>> If we incorrectly compute the tls boundaries, lsan my produce false
>> positives or false negatives.
>
> But is that solely for lsan and nothing else?

Mmm. I *think* yes, today this is lsan-only.

> Because, the assertion
> was failing in asan tests, without any asan options to request leak
> checking.  And for non-i?86/x86_64 you ignore the tls boundaries too.

My patch above should remove the assertion on < 2.13

>
>> Having kThreadDescriptorSize=0 means that we include the stack
>> descriptor in the lsan's root set and thus
>> may miss a leak (with rather low probability). I can live with this.
>>
>> Like this (tested only on my box)?
>
>> --- sanitizer_linux_libcdep.cc  (revision 196375)
>> +++ sanitizer_linux_libcdep.cc  (working copy)
>> @@ -207,12 +207,12 @@
>>
>>  #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)
>> +// This may change between glibc versions, we only support the versions we know
>> +// avout (>= 2.13). For others we set kThreadDescriptorSize to 0.
>> +#if __GLIBC_PREREQ(2, 13)
>>  const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1216, 2304);
>>  #else
>> -const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1168, 2304);
>> +const uptr kThreadDescriptorSize = 0;  // Unknown.
>
> Depends on (as I've asked earlier) on if you need the exact precise
> value or if say conservatively smaller value is fine.  Then you could
> say for glibc >= 2.5 pick the minimum of the values I've gathered.

precise is better, otherwise we may lose leaks.

>
>         Jakub
diff mbox

Patch

Index: sanitizer_linux_libcdep.cc
===================================================================
--- sanitizer_linux_libcdep.cc  (revision 196375)
+++ sanitizer_linux_libcdep.cc  (working copy)
@@ -207,12 +207,12 @@ 

 #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)
+// This may change between glibc versions, we only support the versions we know
+// avout (>= 2.13). For others we set kThreadDescriptorSize to 0.
+#if __GLIBC_PREREQ(2, 13)
 const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1216, 2304);
 #else
-const uptr kThreadDescriptorSize = FIRST_32_SECOND_64(1168, 2304);
+const uptr kThreadDescriptorSize = 0;  // Unknown.
 #endif

 uptr ThreadDescriptorSize() {
@@ -255,7 +255,7 @@ 
   *stk_addr = stack_bottom;
   *stk_size = stack_top - stack_bottom;

-  if (!main) {
+  if (!main && kThreadDescriptorSize) {
     // If stack and tls intersect, make them non-intersecting.
     if (*tls_addr > *stk_addr && *tls_addr < *stk_addr + *stk_size) {
       CHECK_GT(*tls_addr + *tls_size, *stk_addr);
Index: tests/sanitizer_linux_test.cc
===================================================================
--- tests/sanitizer_linux_test.cc       (revision 196375)
+++ tests/sanitizer_linux_test.cc       (working copy)
@@ -224,6 +224,7 @@ 

 TEST(SanitizerLinux, ThreadDescriptorSize) {
   pthread_t tid;
+  if (!ThreadDescriptorSize()) return;
   void *result;
   ASSERT_EQ(0, pthread_create(&tid, 0, thread_descriptor_size_test_func, 0));
   ASSERT_EQ(0, pthread_join(tid, &result));