On Wed, Dec 12, 2012 at 10:16:49PM +0100, Dodji Seketeli wrote:
> Independently of this review, I think it's be interesting to hear
> Kostya's voice on:
>
> Jakub Jelinek <ja...@redhat.com> writes:
>
> > 2) In large-func-test-1.C, I had to stop matching the backtrace after
> > _Znw[jm], because libasan is using the fast but inaccurate backtrace,
> > and while the tests can be easily tweaked to compile with
> > -fno-omit-frame-pointer, we definitely can't rely on libstdc++.so to be
> > built with that option. Most likely it isn't. I repeat that I think
> > that at least for Linux libasan should use the _Unwind* based backtrace
> > at least for the fatal functions (__asan_report* etc.), and perhaps for
> > these malloc wrappers like ::operator new, ::operator new[] and their
> > const std::nothrow_t& variants libasan could intercept them, call
> > malloc and if that returns NULL, call the original corresponding function
> > so that it deals with exceptions, new handler etc.
Yeah, I'd appreciate that too.
> and on:
>
> > 3) deep-thread-stack-1.C fails for me right now with some libasan assertion,
> > Kostya, can you please look at that?
> > AsanThread *t = asanThreadRegistry().GetCurrent();
> > CHECK(t);
> > where it failed on the CHECK, because t was NULL. I've skipped the test for
> > now.
>
> [...]
This one is for the testcase solved right now already by the -lasan -lpthread
linking instead of just -lpthread (and driver adding -lasan afterwards).
We'll need to think about how to tweak the driver to add -lasan early on the
command line, before user passed -l* options.
>
> > --- gcc/testsuite/g++.dg/asan/deep-tail-call-1.C.jj 2012-12-04
> > 20:24:10.000000000 +0100
> > +++ gcc/testsuite/g++.dg/asan/deep-tail-call-1.C 2012-12-05
> > 11:01:48.600443834 +0100
> > @@ -1,21 +1,22 @@
> > -// { dg-do run }
> > +// { dg-do run }
> > // { dg-options "-fno-omit-frame-pointer -fno-optimize-sibling-calls" }
> > // { dg-additional-options "-mno-omit-leaf-frame-pointer" { target {
> > i?86-*-* x86_64-*-* } } }
> > -// { dg-shouldfail "asan" }
> > +// { dg-shouldfail "asan" }
> >
> > int global[10];
> > void __attribute__((noinline)) call4(int i) { global[i+10]++; }
> > void __attribute__((noinline)) call3(int i) { call4(i); }
> > void __attribute__((noinline)) call2(int i) { call3(i); }
> > void __attribute__((noinline)) call1(int i) { call2(i); }
> > -int main(int argc, char **argv) {
> > - call1(argc);
> > +volatile int one = 1;
>
> Just curious, why do we need this variable to be volatile, especially
> since the test is compiled without optimization?
asan.exp tests are torture tests, they iterate over several -O* options,
unless explicitly dg-skip-if skipped. It could be non-volatile with
asm volatile ("" : : : "memory");
or asm volatile ("" : "+m" (one)); or similar too, sure.
I just don't want to rely on argc being one, and the compiler shouldn't know
that one is 1 in the test.
> [...]
>
> The patch looks OK to me in any case.
Thanks.
Jakub
