[PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64

[PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

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

[-- Attachment #2: gcc-65479.patch --]
[-- Type: text/x-patch, Size: 12923 bytes --]

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  <msebor@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  <msebor@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
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
@@ -1,3 +1,11 @@
+2015-04-19  Martin Sebor  <msebor@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.
+
 2015-01-24  Matthias Klose  <doko@ubuntu.com>
 
 	* configure.ac: Move AM_ENABLE_MULTILIB before AC_PROG_CC.
diff --git a/libbacktrace/dwarf.c b/libbacktrace/dwarf.c
index 919b568..e32c468 100644
--- a/libbacktrace/dwarf.c
+++ b/libbacktrace/dwarf.c
@@ -211,6 +211,10 @@ struct line
   const char *filename;
   /* Line number.  */
   int lineno;
+  /* Index of the object in the original array read from the DWARF
+     section, before it has been sorted.  The index makes it possible
+     to use Quicksort and maintain stability.  */
+  int idx;
 };
 
 /* A growable vector of line number information.  This is used while
@@ -940,9 +944,10 @@ unit_addrs_search (const void *vkey, const void *ventry)
     return 0;
 }
 
-/* Sort the line vector by PC.  We want a stable sort here.  We know
-   that the pointers are into the same array, so it is safe to compare
-   them directly.  */
+/* Sort the line vector by PC.  We want a stable sort here to maintain
+   the order of lines for the same PC values.  Since the sequence is
+   being sorted in place, their addresses cannot be relied on to
+   maintain stability.  That is the purpose of the index member.  */
 
 static int
 line_compare (const void *v1, const void *v2)
@@ -954,9 +959,9 @@ line_compare (const void *v1, const void *v2)
     return -1;
   else if (ln1->pc > ln2->pc)
     return 1;
-  else if (ln1 < ln2)
+  else if (ln1->idx < ln2->idx)
     return -1;
-  else if (ln1 > ln2)
+  else if (ln1->idx > ln2->idx)
     return 1;
   else
     return 0;
@@ -1551,6 +1556,7 @@ add_line (struct backtrace_state *state, struct dwarf_data *ddata,
 
   ln->filename = filename;
   ln->lineno = lineno;
+  ln->idx = vec->count;
 
   ++vec->count;
 
@@ -2011,6 +2017,7 @@ read_line_info (struct backtrace_state *state, struct dwarf_data *ddata,
   ln->pc = (uintptr_t) -1;
   ln->filename = NULL;
   ln->lineno = 0;
+  ln->idx = 0;
 
   if (!backtrace_vector_release (state, &vec.vec, error_callback, data))
     goto fail;
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 Sebor  <msebor@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.
+
 2015-04-13  Yury Gribov  <y.gribov@samsung.com>
 
 	PR sanitizer/64839
diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
index 9b99b5b..9b61b85 100644
--- 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
-#if defined(__powerpc__) || defined(__powerpc64__)
-  // PCs are always 4 byte aligned.
-  return pc - 4;
-#elif defined(__sparc__) || defined(__mips__)
-  return pc - 8;
+const unsigned StackTrace::min_insn_bytes =
+#if defined __ia64__
+    // Intel Itanium has 5 byte instructions.
+    5
+#elif defined AVR
+    2
+#elif defined __i386__ || defined __x86_64__ || defined mc68000
+    // Intel x86 and Motorola 68000 have variable instruction sets.
+    1
+#elif defined __s390__ || defined __sh__
+    // System 390 has 2 or 4 byte instructions.
+    // Hitachi SuperH is a 32-bit CPU with 16-bit instructions.
+    2
 #else
-  return pc - 1;
+    // Most instruction sets define instructions whose size in bytes
+    // matches that of the int type.  I.e., 32-bit and 64-bit RISC
+    // CPUs typically have 32-bit instructions, while 16-bit RISC CPUs
+    // have 16 bit instructions.
+    sizeof (int)
 #endif
+    ;
+
+uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
+    // Return a value that represents the address of either the first
+    // byte or some byte past the first byte of the instruction before
+    // the argument.
+    return pc - min_insn_bytes;
 }
 
 uptr StackTrace::GetCurrentPc() {
diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace.h b/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
index 31495cf..a01f676 100644
--- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
+++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
@@ -39,9 +39,18 @@ static const uptr kStackTraceMax = 256;
 struct StackTrace {
   const uptr *trace;
   uptr size;
-
-  StackTrace() : trace(nullptr), size(0) {}
-  StackTrace(const uptr *trace, uptr size) : trace(trace), size(size) {}
+  // Greater than zero of the stack trace was generated in response
+  // to a signal, -1 otherwise.
+  int signaled;
+
+  // Minimum instruction size in bytes. Typically 4 on RISC processors
+  // and 1 on CISC. Used to advance the PC to the previous instruction
+  // in a stack trace and back.
+  static const unsigned min_insn_bytes;
+
+  StackTrace() : trace(nullptr), size(0), signaled(-1) {}
+  StackTrace(const uptr *trace, uptr size, int signaled = -1)
+    : trace(trace), size(size), signaled(signaled) {}
 
   // Prints a symbolized stacktrace, followed by an empty line.
   void Print() const;
diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
index 2d55b73..7282d4b 100644
--- a/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
@@ -29,9 +29,16 @@ void StackTrace::Print() const {
   InternalScopedString frame_desc(GetPageSizeCached() * 2);
   uptr frame_num = 0;
   for (uptr i = 0; i < size && trace[i]; i++) {
-    // PCs in stack traces are actually the return addresses, that is,
-    // addresses of the next instructions after the call.
-    uptr pc = GetPreviousInstructionPc(trace[i]);
+    // Except when the stack trace was generated in response to a signal
+    // and the frame is #0, decrementing the PC in the stack trace by
+    // the size of the smallest instruction helps find the expected
+    // corresponding line number in the debug info.  This is because
+    // with the exception of frame #0 the PC saved on the stack is
+    // the return address, that is, the address of the next instructions
+    // after a branch and link or call instruction.
+    const uptr pc = (0 == i) && signaled ?
+      trace[i] : trace[i] - StackTrace::min_insn_bytes;
+
     uptr addr_frames_num = Symbolizer::GetOrInit()->SymbolizePC(
         pc, addr_frames.data(), kMaxAddrFrames);
     if (addr_frames_num == 0) {
@@ -40,6 +47,10 @@ void StackTrace::Print() const {
     }
     for (uptr j = 0; j < addr_frames_num; j++) {
       AddressInfo &info = addr_frames[j];
+      if (i || !signaled) {
+        // Restore the PC to match the stack trace (see also PR65749).
+        info.address += StackTrace::min_insn_bytes;
+      }
       frame_desc.clear();
       RenderFrame(&frame_desc, common_flags()->stack_trace_format, frame_num++,
                   info, common_flags()->strip_path_prefix);
@@ -54,6 +65,9 @@ void StackTrace::Print() const {
 void BufferedStackTrace::Unwind(uptr max_depth, uptr pc, uptr bp, void *context,
                                 uptr stack_top, uptr stack_bottom,
                                 bool request_fast_unwind) {
+  // Record that the stack trace was generated as a result of a signal.
+  signaled = 0 != context;
+
   top_frame_bp = (max_depth > 0) ? bp : 0;
   // Avoid doing any work for small max_depth.
   if (max_depth == 0) {

Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64

[Jeff Law]

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 Sebor<msebor@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 Sebor<msebor@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 Sebor<msebor@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

[Yury Gribov]

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 Sebor<msebor@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

[Jeff Law]

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

[Jakub Jelinek]

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 Sebor<msebor@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

[Yury Gribov]

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 Sebor<msebor@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

Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 3216 bytes --]

>> 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<msebor@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 Sebor<msebor@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

[-- Attachment #2: gcc-65479.patch --]
[-- Type: text/x-patch, Size: 12949 bytes --]

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  <msebor@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  <msebor@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
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
@@ -1,3 +1,11 @@
+2015-04-19  Martin Sebor  <msebor@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.
+
 2015-01-24  Matthias Klose  <doko@ubuntu.com>
 
 	* configure.ac: Move AM_ENABLE_MULTILIB before AC_PROG_CC.
diff --git a/libbacktrace/dwarf.c b/libbacktrace/dwarf.c
index 919b568..e32c468 100644
--- a/libbacktrace/dwarf.c
+++ b/libbacktrace/dwarf.c
@@ -211,6 +211,10 @@ struct line
   const char *filename;
   /* Line number.  */
   int lineno;
+  /* Index of the object in the original array read from the DWARF
+     section, before it has been sorted.  The index makes it possible
+     to use Quicksort and maintain stability.  */
+  int idx;
 };
 
 /* A growable vector of line number information.  This is used while
@@ -940,9 +944,10 @@ unit_addrs_search (const void *vkey, const void *ventry)
     return 0;
 }
 
-/* Sort the line vector by PC.  We want a stable sort here.  We know
-   that the pointers are into the same array, so it is safe to compare
-   them directly.  */
+/* Sort the line vector by PC.  We want a stable sort here to maintain
+   the order of lines for the same PC values.  Since the sequence is
+   being sorted in place, their addresses cannot be relied on to
+   maintain stability.  That is the purpose of the index member.  */
 
 static int
 line_compare (const void *v1, const void *v2)
@@ -954,9 +959,9 @@ line_compare (const void *v1, const void *v2)
     return -1;
   else if (ln1->pc > ln2->pc)
     return 1;
-  else if (ln1 < ln2)
+  else if (ln1->idx < ln2->idx)
     return -1;
-  else if (ln1 > ln2)
+  else if (ln1->idx > ln2->idx)
     return 1;
   else
     return 0;
@@ -1551,6 +1556,7 @@ add_line (struct backtrace_state *state, struct dwarf_data *ddata,
 
   ln->filename = filename;
   ln->lineno = lineno;
+  ln->idx = vec->count;
 
   ++vec->count;
 
@@ -2011,6 +2017,7 @@ read_line_info (struct backtrace_state *state, struct dwarf_data *ddata,
   ln->pc = (uintptr_t) -1;
   ln->filename = NULL;
   ln->lineno = 0;
+  ln->idx = 0;
 
   if (!backtrace_vector_release (state, &vec.vec, error_callback, data))
     goto fail;
diff --git a/libsanitizer/ChangeLog b/libsanitizer/ChangeLog
index 6f44dcf..7b82378 100644
--- a/libsanitizer/ChangeLog
+++ b/libsanitizer/ChangeLog
@@ -1,3 +1,16 @@
+2015-04-19  Martin Sebor  <msebor@redhat.com>
+
+	PR sanitizer/65479
+	PR sanitizer/65749
+	* 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.
+
 2015-04-13  Yury Gribov  <y.gribov@samsung.com>
 
 	PR sanitizer/64839
diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
index 9b99b5b..9b61b85 100644
--- 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
-#if defined(__powerpc__) || defined(__powerpc64__)
-  // PCs are always 4 byte aligned.
-  return pc - 4;
-#elif defined(__sparc__) || defined(__mips__)
-  return pc - 8;
+const unsigned StackTrace::min_insn_bytes =
+#if defined __ia64__
+    // Intel Itanium has 5 byte instructions.
+    5
+#elif defined AVR
+    2
+#elif defined __i386__ || defined __x86_64__ || defined mc68000
+    // Intel x86 and Motorola 68000 have variable instruction sets.
+    1
+#elif defined __s390__ || defined __sh__
+    // System 390 has 2 or 4 byte instructions.
+    // Hitachi SuperH is a 32-bit CPU with 16-bit instructions.
+    2
 #else
-  return pc - 1;
+    // Most instruction sets define instructions whose size in bytes
+    // matches that of the int type.  I.e., 32-bit and 64-bit RISC
+    // CPUs typically have 32-bit instructions, while 16-bit RISC CPUs
+    // have 16 bit instructions.
+    sizeof (int)
 #endif
+    ;
+
+uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
+    // Return a value that represents the address of either the first
+    // byte or some byte past the first byte of the instruction before
+    // the argument.
+    return pc - min_insn_bytes;
 }
 
 uptr StackTrace::GetCurrentPc() {
diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace.h b/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
index 31495cf..a01f676 100644
--- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
+++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
@@ -39,9 +39,18 @@ static const uptr kStackTraceMax = 256;
 struct StackTrace {
   const uptr *trace;
   uptr size;
-
-  StackTrace() : trace(nullptr), size(0) {}
-  StackTrace(const uptr *trace, uptr size) : trace(trace), size(size) {}
+  // Greater than zero of the stack trace was generated in response
+  // to a signal, -1 otherwise.
+  int signaled;
+
+  // Minimum instruction size in bytes. Typically 4 on RISC processors
+  // and 1 on CISC. Used to advance the PC to the previous instruction
+  // in a stack trace and back.
+  static const unsigned min_insn_bytes;
+
+  StackTrace() : trace(nullptr), size(0), signaled(-1) {}
+  StackTrace(const uptr *trace, uptr size, int signaled = -1)
+    : trace(trace), size(size), signaled(signaled) {}
 
   // Prints a symbolized stacktrace, followed by an empty line.
   void Print() const;
diff --git a/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
index 2d55b73..7282d4b 100644
--- a/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
@@ -29,9 +29,16 @@ void StackTrace::Print() const {
   InternalScopedString frame_desc(GetPageSizeCached() * 2);
   uptr frame_num = 0;
   for (uptr i = 0; i < size && trace[i]; i++) {
-    // PCs in stack traces are actually the return addresses, that is,
-    // addresses of the next instructions after the call.
-    uptr pc = GetPreviousInstructionPc(trace[i]);
+    // Except when the stack trace was generated in response to a signal
+    // and the frame is #0, decrementing the PC in the stack trace by
+    // the size of the smallest instruction helps find the expected
+    // corresponding line number in the debug info.  This is because
+    // with the exception of frame #0 the PC saved on the stack is
+    // the return address, that is, the address of the next instructions
+    // after a branch and link or call instruction.
+    const uptr pc = (0 == i) && signaled ?
+      trace[i] : trace[i] - StackTrace::min_insn_bytes;
+
     uptr addr_frames_num = Symbolizer::GetOrInit()->SymbolizePC(
         pc, addr_frames.data(), kMaxAddrFrames);
     if (addr_frames_num == 0) {
@@ -40,6 +47,10 @@ void StackTrace::Print() const {
     }
     for (uptr j = 0; j < addr_frames_num; j++) {
       AddressInfo &info = addr_frames[j];
+      if (i || !signaled) {
+        // Restore the PC to match the stack trace (see also PR65749).
+        info.address += StackTrace::min_insn_bytes;
+      }
       frame_desc.clear();
       RenderFrame(&frame_desc, common_flags()->stack_trace_format, frame_num++,
                   info, common_flags()->strip_path_prefix);
@@ -54,6 +65,9 @@ void StackTrace::Print() const {
 void BufferedStackTrace::Unwind(uptr max_depth, uptr pc, uptr bp, void *context,
                                 uptr stack_top, uptr stack_bottom,
                                 bool request_fast_unwind) {
+  // Record that the stack trace was generated as a result of a signal.
+  signaled = 0 != context;
+
   top_frame_bp = (max_depth > 0) ? bp : 0;
   // Avoid doing any work for small max_depth.
   if (max_depth == 0) {

Re: [PATCH] 65479 - sanitizer stack trace missing frames past #0 on powerpc64

[Jakub Jelinek]

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  <msebor@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

[Peter Bergner]

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

[Martin Sebor]

>> --- a/libsanitizer/ChangeLog
>> +++ b/libsanitizer/ChangeLog
>> @@ -1,3 +1,15 @@
>> +2015-04-19  Martin Sebor  <msebor@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

[Martin Sebor]

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

[Jeff Law]

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

[Martin Sebor]

> 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

[Jeff Law]

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

[Martin Sebor]

> 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