This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 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
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) {

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]