Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
OK. You can go ahead and commit the libbacktrace fix. Committed in r224402. Let's hold off on the testsuite fixes until we've got the sanitizer libbacktrace fixes installed. Okay. Martin
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 05/20/2015 05:01 PM, Martin Sebor wrote: Now that I know a bit more from Jakub Yuri's comments, I don't think 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. Thanks. Seems to be moving along. Looks like they were a bit surprised that you'd included a patch ;-) 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. OK. You can go ahead and commit the libbacktrace fix. Let's hold off on the testsuite fixes until we've got the sanitizer libbacktrace fixes installed. jeff
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 04/20/2015 04:32 PM, Martin Sebor wrote: I also wonder if other targets need -fasynchronous-unwind-tables and whether or not we should just add it unconditionally. I initially only tested powerpc64* and x86_64. I had tried s370 but asan doesn't appear to be built there (is it not supported?) I've now also tried aarch64 (partly because the patch also fixes a latent bug there). Of these targets, only powerpc64* needs the option, and only until the fast unwinding that Jakub mentioned is implemented. I plan to work on it but I wanted to get this simpler fix in first if only so there is a working baseline to start from. Sorry for the deep context switch asan is only supported on a few platforms and I'm pretty sure s390 isn't one of them. 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. Is libsanitizer maintained in LLVM? If so, we want to minimize divergence, so it may be better to get this approved in LLVM then pick it up via a merge. I can certainly see about submitting the sanitizer bits of the patch to LLVM. It will probably take some time and I was hoping for cleaner powerpc test results in 5.0 (or 5.1 as it sounds like the release will be called). I don't yet have a sense of whether it's preferable to do one or the other or whether it makes sense to do both (i.e., commit the fix now and then merge). This part should definitely hit the LLVM side first, then we can pull it into GCC. So that process should be started. Given this hits 3 distinct pieces of code, do any of them make sense in isolation or do they have to land together as a unit? 65479 (this bug) depends on 65749 (sanitizer stack trace pc off by 1). The asan tests cannot very well be made to pass without fixing the latter bug. Let me know how you'd like to proceed with the patch. That's part of what I was trying to figure out myself :-) 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. 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). Thoughts? jeff
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
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
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
Hi!
Note, the changes aren't acceptable for 5.1 at this point (release is
tomorrow), and for 5.2 backport they should be tested for a while on the
trunk, so there is no rush now.
--- a/libsanitizer/ChangeLog
+++ b/libsanitizer/ChangeLog
@@ -1,3 +1,15 @@
+2015-04-19 Martin Sebor mse...@redhat.com
+
+ PR sanitizer/65479
+ * libsanitizer/sanitizer_common/sanitizer_stacktrace.h
+ (StackTrace::signaled, StackTrace::min_insn_bytes): New data members.
+ (StackTrace::StackTrace): Initialize signaled.
+ * libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
+ (StackTrace::GetPreviousInstructionPc): Rewrite.
+ * libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
+ (StackTrace::Print): Use min_insn_bytes to adjust PC value.
+ (BufferedStackTrace::Unwind): Set signaled.
libsanitizer/ should not show up in the ChangeLog entry.
But as somebody said earlier, the libsanitizer changes really should go
to LLVM compiler-rt repo first and then be just backported, either
cherry-picked (probably the case for the 5 branch backport later on) or go in
full merge from compiler-rt.
--- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
@@ -15,19 +15,33 @@
namespace __sanitizer {
-uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
-#if defined(__arm__)
- // Cancel Thumb bit.
- pc = pc (~1);
-#endif
Your code loses this, which is undesirable.
-#if defined(__powerpc__) || defined(__powerpc64__)
- // PCs are always 4 byte aligned.
- return pc - 4;
-#elif defined(__sparc__) || defined(__mips__)
- return pc - 8;
The SPARC/MIPS case is of course needed, because on these architectures
the call is followed by a delay slot. But I wonder why you need anything
special on any other architecture, why pc - 1 isn't good enough for those.
The point isn't to find a PC of the call instruction, on some targets that
is very hard and you need to disassemble, but to just find some byte in the
call instruction.
+const unsigned StackTrace::min_insn_bytes =
+#if defined __ia64__
+// Intel Itanium has 5 byte instructions.
+5
E.g. this is wrong, ia64 doesn't have 5 byte instructions, but has VLIW
bundles, where in the 16 byte bundle there are up to 3 41-bit instructions
plus template. But, ia64 isn't supported by libsanitizer and I doubt there
are enough users that would be interested in writing support for a dead
architecture.
Jakub
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 04/21/2015 06:39 AM, Peter Bergner wrote: On Tue, 2015-04-21 at 08:22 +0200, Jakub Jelinek wrote: -#if defined(__powerpc__) || defined(__powerpc64__) - // PCs are always 4 byte aligned. - return pc - 4; -#elif defined(__sparc__) || defined(__mips__) - return pc - 8; The SPARC/MIPS case is of course needed, because on these architectures the call is followed by a delay slot. But I wonder why you need anything special on any other architecture, why pc - 1 isn't good enough for those. The point isn't to find a PC of the call instruction, on some targets that is very hard and you need to disassemble, but to just find some byte in the call instruction. I wrote the pc - 4 code for powerpc* and I guess I was just being pedantic on returning the first address of the instruction. If using pc - 1 works, then I'm fine with that. It works fine with the patch and produces sensible output because the decremented address is only used to look up the debug info and restored before it's output. Otherwise (with the unpatched code) we'd end up with odd PC addresses in the stack trace. Martin Peter
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
--- a/libsanitizer/ChangeLog
+++ b/libsanitizer/ChangeLog
@@ -1,3 +1,15 @@
+2015-04-19 Martin Sebor mse...@redhat.com
+
+ PR sanitizer/65479
+ * libsanitizer/sanitizer_common/sanitizer_stacktrace.h
+ (StackTrace::signaled, StackTrace::min_insn_bytes): New data members.
+ (StackTrace::StackTrace): Initialize signaled.
+ * libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
+ (StackTrace::GetPreviousInstructionPc): Rewrite.
+ * libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
+ (StackTrace::Print): Use min_insn_bytes to adjust PC value.
+ (BufferedStackTrace::Unwind): Set signaled.
libsanitizer/ should not show up in the ChangeLog entry.
But as somebody said earlier, the libsanitizer changes really should go
to LLVM compiler-rt repo first and then be just backported, either
cherry-picked (probably the case for the 5 branch backport later on) or go in
full merge from compiler-rt.
Okay, let me submit the sanitizer changes there. Since the
tests will continue to fail without it, the libbacktrace
change can go in later if that's preferable.
--- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
@@ -15,19 +15,33 @@
namespace __sanitizer {
-uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
-#if defined(__arm__)
- // Cancel Thumb bit.
- pc = pc (~1);
-#endif
Your code loses this, which is undesirable.
The original function fails to return the pc value on ARM
so I just took it out. I didn't look into what the intent
was but all the tests pass with the patch on aarch64 (after
applying the Fedora gcc 5 patch you mentioned yesterday).
-#if defined(__powerpc__) || defined(__powerpc64__)
- // PCs are always 4 byte aligned.
- return pc - 4;
-#elif defined(__sparc__) || defined(__mips__)
- return pc - 8;
The SPARC/MIPS case is of course needed, because on these architectures
the call is followed by a delay slot. But I wonder why you need anything
special on any other architecture, why pc - 1 isn't good enough for those.
The point isn't to find a PC of the call instruction, on some targets that
is very hard and you need to disassemble, but to just find some byte in the
call instruction.
I forgot about the delay slot. Thanks for the reminder.
+const unsigned StackTrace::min_insn_bytes =
+#if defined __ia64__
+// Intel Itanium has 5 byte instructions.
+5
E.g. this is wrong, ia64 doesn't have 5 byte instructions, but has VLIW
bundles, where in the 16 byte bundle there are up to 3 41-bit instructions
plus template. But, ia64 isn't supported by libsanitizer and I doubt there
are enough users that would be interested in writing support for a dead
architecture.
I suppose with the sanitizer output referencing the unmodified
PC values on the stack the computation can be simplified to
just subtract (and later add) 1 on all targets. Let me change
that.
Martin
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On Tue, 2015-04-21 at 08:22 +0200, Jakub Jelinek wrote: -#if defined(__powerpc__) || defined(__powerpc64__) - // PCs are always 4 byte aligned. - return pc - 4; -#elif defined(__sparc__) || defined(__mips__) - return pc - 8; The SPARC/MIPS case is of course needed, because on these architectures the call is followed by a delay slot. But I wonder why you need anything special on any other architecture, why pc - 1 isn't good enough for those. The point isn't to find a PC of the call instruction, on some targets that is very hard and you need to disassemble, but to just find some byte in the call instruction. I wrote the pc - 4 code for powerpc* and I guess I was just being pedantic on returning the first address of the instruction. If using pc - 1 works, then I'm fine with that. Peter
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index b4052ef..18eede3 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,12 @@
+2015-04-19 Martin Sebormse...@redhat.com
+
+PR sanitizer/65479
+* gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]:
+Use -fno-omit-frame-pointer. Adjust line numbers and expect exact
+matches.
+* gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto.
+* gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto.
So the ChangeLog doesn't match the patch. The changelog references
-fno-omit-frame-pointer, but in the patch you actually add
-fasynchronous-unwind-tables.
Thanks for the review. This incorrect option is a copy paste
typo in the ChangeLog entry. The patch is correct as is. I've
corrected the ChangeLog in the new version of the patch
(attached) where I mention that the patch also fixes pr65749.
I also wonder if other targets need -fasynchronous-unwind-tables and
whether or not we should just add it unconditionally.
I initially only tested powerpc64* and x86_64. I had tried s370
but asan doesn't appear to be built there (is it not supported?)
I've now also tried aarch64 (partly because the patch also fixes
a latent bug there). Of these targets, only powerpc64* needs
the option, and only until the fast unwinding that Jakub
mentioned is implemented. I plan to work on it but I wanted to
get this simpler fix in first if only so there is a working
baseline to start from.
diff --git a/libsanitizer/ChangeLog b/libsanitizer/ChangeLog
index 6f44dcf..7b82378 100644
--- a/libsanitizer/ChangeLog
+++ b/libsanitizer/ChangeLog
@@ -1,3 +1,15 @@
+2015-04-19 Martin Sebormse...@redhat.com
+
+PR sanitizer/65479
+* libsanitizer/sanitizer_common/sanitizer_stacktrace.h
+(StackTrace::signaled, StackTrace::min_insn_bytes): New data
members.
+(StackTrace::StackTrace): Initialize signaled.
+* libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
+(StackTrace::GetPreviousInstructionPc): Rewrite.
+* libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
+(StackTrace::Print): Use min_insn_bytes to adjust PC value.
+(BufferedStackTrace::Unwind): Set signaled.
Is libsanitizer maintained in LLVM? If so, we want to minimize
divergence, so it may be better to get this approved in LLVM then pick
it up via a merge.
I can certainly see about submitting the sanitizer bits of
the patch to LLVM. It will probably take some time and I
was hoping for cleaner powerpc test results in 5.0 (or 5.1
as it sounds like the release will be called). I don't yet
have a sense of whether it's preferable to do one or the
other or whether it makes sense to do both (i.e., commit
the fix now and then merge).
Given this hits 3 distinct pieces of code, do any of them make sense in
isolation or do they have to land together as a unit?
65479 (this bug) depends on 65749 (sanitizer stack trace
pc off by 1). The asan tests cannot very well be made to
pass without fixing the latter bug.
Let me know how you'd like to proceed with the patch.
Martin
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index b4052ef..18eede3 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,12 @@
+2015-04-19 Martin Sebor mse...@redhat.com
+
+ PR sanitizer/65479
+ * gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]:
+ Use -fasynchronous-unwind-tables. Adjust line numbers and expect exact
+ matches.
+ * gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto.
+ * gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto.
+
2015-04-18 Martin Sebor mse...@redhat.com
* gfortran.dg/pr32627.f03 (strptr): Change size to match the number
diff --git a/gcc/testsuite/c-c++-common/asan/misalign-1.c b/gcc/testsuite/c-c++-common/asan/misalign-1.c
index f1cca16..833b82a 100644
--- a/gcc/testsuite/c-c++-common/asan/misalign-1.c
+++ b/gcc/testsuite/c-c++-common/asan/misalign-1.c
@@ -1,5 +1,6 @@
/* { dg-do run { target { ilp32 || lp64 } } } */
/* { dg-options -O2 } */
+/* { dg-additional-options -fasynchronous-unwind-tables { target powerpc*-*-*-* } } */
/* { dg-additional-options -fno-omit-frame-pointer { target *-*-darwin* } } */
/* { dg-shouldfail asan } */
@@ -39,5 +40,5 @@ main ()
/* { dg-output ERROR: AddressSanitizer:\[^\n\r]*on address\[^\n\r]* } */
/* { dg-output 0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r) } */
/* { dg-output \[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #0 0x\[0-9a-f\]+ +(in _*foo(\[^\n\r]*misalign-1.c:1\[01]|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*misalign-1.c:3\[45]|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r) } */
+/* { dg-output #0 0x\[0-9a-f\]+ +(in _*foo(\[^\n\r]*misalign-1.c:12|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
+/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 04/19/2015 07:48 PM, Martin Sebor wrote: The attached patch resolves the failures in a number of address sanitizer tests on powerpc64*-*-*-* discussed in bug 65479 (the failures in c-c++-common/asan/swapcontext-test-1.c reported in pr65643 remain unresolved). The patch has been tested on powerpc64*-*-*-* and x86_64 with no regressions. Is this okay for trunk? For 5.1? Martin gcc-65479.patch diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index b4052ef..18eede3 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,12 @@ +2015-04-19 Martin Sebormse...@redhat.com + + PR sanitizer/65479 + * gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]: + Use -fno-omit-frame-pointer. Adjust line numbers and expect exact + matches. + * gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto. + * gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto. So the ChangeLog doesn't match the patch. The changelog references -fno-omit-frame-pointer, but in the patch you actually add -fasynchronous-unwind-tables. I also wonder if other targets need -fasynchronous-unwind-tables and whether or not we should just add it unconditionally. diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog index e385d8f..9348321 100644 --- a/libbacktrace/ChangeLog +++ b/libbacktrace/ChangeLog @@ -1,3 +1,11 @@ +2015-04-19 Martin Sebormse...@redhat.com + + PR sanitizer/65479 + * libbacktrace/dwarf.c (struct line): Add idx data member. + (line_compare): Use struct line idx member. + (add_line): Set ln-idx. + (read_line_info): Clear ln-idx. Seems OK. diff --git a/libsanitizer/ChangeLog b/libsanitizer/ChangeLog index 6f44dcf..7b82378 100644 --- a/libsanitizer/ChangeLog +++ b/libsanitizer/ChangeLog @@ -1,3 +1,15 @@ +2015-04-19 Martin Sebormse...@redhat.com + + PR sanitizer/65479 + * libsanitizer/sanitizer_common/sanitizer_stacktrace.h + (StackTrace::signaled, StackTrace::min_insn_bytes): New data members. + (StackTrace::StackTrace): Initialize signaled. + * libsanitizer/sanitizer_common/sanitizer_stacktrace.cc + (StackTrace::GetPreviousInstructionPc): Rewrite. + * libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc + (StackTrace::Print): Use min_insn_bytes to adjust PC value. + (BufferedStackTrace::Unwind): Set signaled. Is libsanitizer maintained in LLVM? If so, we want to minimize divergence, so it may be better to get this approved in LLVM then pick it up via a merge. Given this hits 3 distinct pieces of code, do any of them make sense in isolation or do they have to land together as a unit? Jeff
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On Mon, Apr 20, 2015 at 09:38:03PM +0300, Yury Gribov wrote: --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,12 @@ +2015-04-19 Martin Sebormse...@redhat.com + +PR sanitizer/65479 +* gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]: +Use -fno-omit-frame-pointer. Adjust line numbers and expect exact +matches. +* gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto. +* gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto. So the ChangeLog doesn't match the patch. The changelog references -fno-omit-frame-pointer, but in the patch you actually add -fasynchronous-unwind-tables. I also wonder if other targets need -fasynchronous-unwind-tables and whether or not we should just add it unconditionally. PowerPC really should use the fast unwinding unconditionally, as it always works there reliably due to the ABI requirements. So IMHO we shouldn't change the tests this way. Perhaps enable unwind tables in GCC spec if -fsanitize=address is present? No. That is orthogonal to that, most targets enable them by default anyway and if somebody for some reason asks for something different, we should honor that. Sanitizer backtraces typically won't work without unwind tables anyway so IMHO this makes sense. BTW why do we need asynchronous tables? Wouldn't simple -funwind-tables be enough? -funwind-tables enables them only for functions that can throw, while you really want it for all functions. Jakub
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 04/20/2015 09:23 PM, Jeff Law wrote: On 04/19/2015 07:48 PM, Martin Sebor wrote: The attached patch resolves the failures in a number of address sanitizer tests on powerpc64*-*-*-* discussed in bug 65479 (the failures in c-c++-common/asan/swapcontext-test-1.c reported in pr65643 remain unresolved). The patch has been tested on powerpc64*-*-*-* and x86_64 with no regressions. Is this okay for trunk? For 5.1? Martin gcc-65479.patch diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index b4052ef..18eede3 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,12 @@ +2015-04-19 Martin Sebormse...@redhat.com + +PR sanitizer/65479 +* gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]: +Use -fno-omit-frame-pointer. Adjust line numbers and expect exact +matches. +* gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto. +* gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto. So the ChangeLog doesn't match the patch. The changelog references -fno-omit-frame-pointer, but in the patch you actually add -fasynchronous-unwind-tables. I also wonder if other targets need -fasynchronous-unwind-tables and whether or not we should just add it unconditionally. Perhaps enable unwind tables in GCC spec if -fsanitize=address is present? Sanitizer backtraces typically won't work without unwind tables anyway so IMHO this makes sense. BTW why do we need asynchronous tables? Wouldn't simple -funwind-tables be enough? -Y
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 04/20/2015 12:38 PM, Yury Gribov wrote: Perhaps enable unwind tables in GCC spec if -fsanitize=address is present? Sanitizer backtraces typically won't work without unwind tables anyway so IMHO this makes sense. BTW why do we need asynchronous tables? Wouldn't simple -funwind-tables be enough? I haven't thought much about it. I'd kind of expect with the instrumentation for the sanitizers that it wouldn't make much, if any difference. But I've never been inside the sanitizer instrumentation :) jeff
Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
On 04/20/2015 09:43 PM, Jakub Jelinek wrote: On Mon, Apr 20, 2015 at 09:38:03PM +0300, Yury Gribov wrote: --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,12 @@ +2015-04-19 Martin Sebormse...@redhat.com + +PR sanitizer/65479 +* gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]: +Use -fno-omit-frame-pointer. Adjust line numbers and expect exact +matches. +* gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto. +* gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto. So the ChangeLog doesn't match the patch. The changelog references -fno-omit-frame-pointer, but in the patch you actually add -fasynchronous-unwind-tables. I also wonder if other targets need -fasynchronous-unwind-tables and whether or not we should just add it unconditionally. PowerPC really should use the fast unwinding unconditionally, as it always works there reliably due to the ABI requirements. So IMHO we shouldn't change the tests this way. Agreed, I think Martin just wanted a temp workaround until he gets to fix PowerPC unwinder in LLVM. Perhaps enable unwind tables in GCC spec if -fsanitize=address is present? No. That is orthogonal to that, most targets enable them by default anyway and if somebody for some reason asks for something different, we should honor that. Sanitizer backtraces typically won't work without unwind tables anyway so IMHO this makes sense. BTW why do we need asynchronous tables? Wouldn't simple -funwind-tables be enough? -funwind-tables enables them only for functions that can throw, while you really want it for all functions. Right but asynchronous tables also enable them for all instructions which is quite an overkill. -Y
[PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64
The attached patch resolves the failures in a number of address
sanitizer tests on powerpc64*-*-*-* discussed in bug 65479 (the
failures in c-c++-common/asan/swapcontext-test-1.c reported in
pr65643 remain unresolved).
The patch has been tested on powerpc64*-*-*-* and x86_64 with
no regressions.
Is this okay for trunk? For 5.1?
Martin
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index b4052ef..18eede3 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,12 @@
+2015-04-19 Martin Sebor mse...@redhat.com
+
+ PR sanitizer/65479
+ * gcc/testsuite/c-c++-common/asan/misalign-1.c [powerpc*-*-*-*]:
+ Use -fno-omit-frame-pointer. Adjust line numbers and expect exact
+ matches.
+ * gcc/testsuite/c-c++-common/asan/misalign-2.c: Ditto.
+ * gcc/testsuite/c-c++-common/asan/null-deref-1.c: Ditto.
+
2015-04-18 Martin Sebor mse...@redhat.com
* gfortran.dg/pr32627.f03 (strptr): Change size to match the number
diff --git a/gcc/testsuite/c-c++-common/asan/misalign-1.c b/gcc/testsuite/c-c++-common/asan/misalign-1.c
index f1cca16..833b82a 100644
--- a/gcc/testsuite/c-c++-common/asan/misalign-1.c
+++ b/gcc/testsuite/c-c++-common/asan/misalign-1.c
@@ -1,5 +1,6 @@
/* { dg-do run { target { ilp32 || lp64 } } } */
/* { dg-options -O2 } */
+/* { dg-additional-options -fasynchronous-unwind-tables { target powerpc*-*-*-* } } */
/* { dg-additional-options -fno-omit-frame-pointer { target *-*-darwin* } } */
/* { dg-shouldfail asan } */
@@ -39,5 +40,5 @@ main ()
/* { dg-output ERROR: AddressSanitizer:\[^\n\r]*on address\[^\n\r]* } */
/* { dg-output 0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r) } */
/* { dg-output \[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #0 0x\[0-9a-f\]+ +(in _*foo(\[^\n\r]*misalign-1.c:1\[01]|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*misalign-1.c:3\[45]|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r) } */
+/* { dg-output #0 0x\[0-9a-f\]+ +(in _*foo(\[^\n\r]*misalign-1.c:12|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
+/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*misalign-1.c:36|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r) } */
diff --git a/gcc/testsuite/c-c++-common/asan/misalign-2.c b/gcc/testsuite/c-c++-common/asan/misalign-2.c
index 9f400b4..923d26b 100644
--- a/gcc/testsuite/c-c++-common/asan/misalign-2.c
+++ b/gcc/testsuite/c-c++-common/asan/misalign-2.c
@@ -1,5 +1,6 @@
/* { dg-do run { target { ilp32 || lp64 } } } */
/* { dg-options -O2 } */
+/* { dg-additional-options -fasynchronous-unwind-tables { target powerpc*-*-*-* } } */
/* { dg-additional-options -fno-omit-frame-pointer { target *-*-darwin* } } */
/* { dg-shouldfail asan } */
@@ -39,5 +40,5 @@ main ()
/* { dg-output ERROR: AddressSanitizer:\[^\n\r]*on address\[^\n\r]* } */
/* { dg-output 0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r) } */
/* { dg-output \[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #0 0x\[0-9a-f\]+ +(in _*baz(\[^\n\r]*misalign-2.c:2\[23]|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*misalign-2.c:3\[45]|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r) } */
+/* { dg-output #0 0x\[0-9a-f\]+ +(in _*baz(\[^\n\r]*misalign-2.c:24|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
+/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*misalign-2.c:36|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r) } */
diff --git a/gcc/testsuite/c-c++-common/asan/null-deref-1.c b/gcc/testsuite/c-c++-common/asan/null-deref-1.c
index 45d35ac..b9bc3e5 100644
--- a/gcc/testsuite/c-c++-common/asan/null-deref-1.c
+++ b/gcc/testsuite/c-c++-common/asan/null-deref-1.c
@@ -1,5 +1,6 @@
/* { dg-do run } */
/* { dg-options -fno-omit-frame-pointer -fno-shrink-wrap } */
+/* { dg-additional-options -fasynchronous-unwind-tables { target { powerpc*-*-*-*} } } */
/* { dg-additional-options -mno-omit-leaf-frame-pointer { target { i?86-*-* x86_64-*-* } } } */
/* { dg-shouldfail asan } */
@@ -18,5 +19,5 @@ int main()
/* { dg-output ERROR: AddressSanitizer:? SEGV on unknown address\[^\n\r]* } */
/* { dg-output 0x\[0-9a-f\]+ \[^\n\r]*pc 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output \[^\n\r]*#0 0x\[0-9a-f\]+ +(in \[^\n\r]*NullDeref\[^\n\r]* (\[^\n\r]*null-deref-1.c:10|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
-/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*null-deref-1.c:15|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
+/* { dg-output \[^\n\r]*#0 0x\[0-9a-f\]+ +(in \[^\n\r]*NullDeref\[^\n\r]* (\[^\n\r]*null-deref-1.c:11|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
+/* { dg-output #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*null-deref-1.c:16|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r) } */
diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index e385d8f..9348321 100644
---
