Now that I know a bit more from Jakub & Yuri's comments, I don't think
we should be turning that flag on for all the targets in the testsuite.
  I was primarily trying to avoid someone else having to go through the
same analysis and reach the same conclusion for another port.  But I'm
less concerned about that now.

I totally understand the desire to have a good baseline.  Jakub seems to
prefer not to make this change since it's a short-lived workaround,
which I understand as well.

My inclination is to go ahead with flags changes in the testsuite.
Cleaner results are, in and of themselves, a good thing.  Pulling those
lines out once the port is using the fast unwind stuff is easy enough to
do.

Adding the flags without making any of the other changes will
not improve the pass rate. The tests hardcode the line numbers
expected to be printed in the stack trace and those are wrong
as a result of both the libbacktrace bug and the ASan bug. We
need to fix both of those to eliminate the test failures.

This part should definitely hit the LLVM side first, then we can pull it
into GCC.  So that process should be started.

I opened compiler_rt/23600 and attached the sanitizer patch
to it:

  http://llvm.org/bugs/show_bug.cgi?id=23600

If there are no objections to the bug I'll post the patch for
review next.

So I'll ask the question(s) in a slightly different way and see if that
guides us one direction or another.

Do the libbacktrace changes make sense independently?  ie, are they the
right thing to do, even if they don't fix a visible bug?  ISTM the
answer to both questions is yes.  In which case, that part ought to go
forward now rather than waiting.

Yes. They fix the sort stability bug. The bug can be reproduced
without any ASan patches by compiling the ASan tests on powerpc64
with the -fasynchronous-unwind-tables option. It's also possible
that the bug can be reproduced with other programs on other
targets. It's just not triggered by the same tests on the targets
we commonly test.


The testsuite changes have two components.  One is the new flag the
other is some slight tweaks to the expected output.  I'd hazard a guess
that the expected output changes ought to go forward independently too.
  Again under the same "it's the right thing to do, even if it doesn't
fix a visible bug".

The testsuite flags change isn't as clear cut.  I'd think they'd need to
visibly improve the test results before they could go in.  So they may
need to wait (I'm assuming nothing actually shows visible improvement
without the libsanitizer fixes).

Correct. To improve the test pass rate on powerpc64 we need all
the fixes (ASan, libbacktrace, the flags or fast unwinding, and
the test output adjustments to reflect the adjusted and tightened
up line numbers in the stack trace).


Thoughts?

I'm fine with whatever approach you all decide on but since
there is a strong preference to have the fix in LLVM first and
since the tests depend on that fix it seems the other changes
need to wait until the ASan patch has been merged from LLVM
to GCC. (None of them can be made independently without
breaking some tests on some targets.)

Martin

Reply via email to