Skip to content

Commit

Permalink
Fix incorrect (or infinite loop) unwind on RHEL7 x86 32 bits.
Browse files Browse the repository at this point in the history
On RHEL7 x86 32 bits, Valgrind unwinder cannot properly unwind
the stack just after a thread creation : the unwinder always retrieves
the same pc/sp/bp.
See below for an example.
This has as consequences that some stack traces are bigger than
needed (i.e. they always fill up the ips array). If
--merge-recursive-frames is given, then the unwinder enters in an
infinite loop (as identical frames will be merged, and the ips array
will never be filled in).
Thi patch adds an additional exit condition : after unwinding
a frame, if the previous sp is >= new sp, then unwinding stops.
Patch has been tested on debian 8/x86, RHEL7/x86.



   0x0417db67 <+55>:    mov    0x18(%esp),%ebx
   0x0417db6b <+59>:    mov    0x28(%esp),%edi
   0x0417db6f <+63>:    mov    $0x78,%eax
   0x0417db74 <+68>:    mov    %ebx,(%ecx)
   0x0417db76 <+70>:    int    $0x80
=> 0x0417db78 <+72>:    pop    %edi
   0x0417db79 <+73>:    pop    %esi
   0x0417db7a <+74>:    pop    %ebx
   0x0417db7b <+75>:    test   %eax,%eax

Valgrind stacktrace gives:
==21261==    at 0x417DB78: clone (clone.S:110)
==21261==    by 0x424702F: ???
==21261==    by 0x424702F: ???
==21261==    by 0x424702F: ???
==21261==    by 0x424702F: ???
==21261==    by 0x424702F: ???
==21261==    by 0x424702F: ???
==21261==    by 0x424702F: ???
...
(till the array of ips is full)

while gdb stacktrace gives:
(gdb) bt
#0  clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:110
svn2github#1  0x00000000 in ?? ()
(gdb) p $pc
$2 = (void (*)()) 0x417db78 <clone+72>
(gdb)


With the fix, valgrind gives:
==21261==    at 0x417DB78: clone (clone.S:110)
==21261==    by 0x424702F: ???
which looks more reasonable.




git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15729 a5019735-40e9-0310-863c-91ae7b9d1cf9
  • Loading branch information
philippe committed Nov 18, 2015
1 parent a9f7ccd commit bd90e82
Showing 1 changed file with 15 additions and 0 deletions.
15 changes: 15 additions & 0 deletions coregrind/m_stacktrace.c
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,8 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
uregs.xbp <= fp_max - 1 * sizeof(UWord)/*see comment below*/ &&
VG_IS_4_ALIGNED(uregs.xbp))
{
Addr old_xsp;

/* fp looks sane, so use it. */
uregs.xip = (((UWord*)uregs.xbp)[1]);
// We stop if we hit a zero (the traditional end-of-stack
Expand Down Expand Up @@ -382,6 +384,7 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
}
}

old_xsp = uregs.xsp;
uregs.xsp = uregs.xbp + sizeof(Addr) /*saved %ebp*/
+ sizeof(Addr) /*ra*/;
uregs.xbp = (((UWord*)uregs.xbp)[0]);
Expand All @@ -393,6 +396,12 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
if (debug) VG_(printf)(" cache FPUNWIND >2\n");
if (debug) unwind_case = "FO";
if (do_stats) stats.FO++;
if (old_xsp >= uregs.xsp) {
if (debug)
VG_(printf) (" FO end of stack old_xsp %p >= xsp %p\n",
(void*)old_xsp, (void*)uregs.xsp);
break;
}
} else {
fp_CF_verif_cache [hash] = xip_verified ^ CFUNWIND;
if (debug) VG_(printf)(" cache CFUNWIND >2\n");
Expand All @@ -406,6 +415,12 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known,
} else {
if (debug) unwind_case = "FF";
if (do_stats) stats.FF++;
if (old_xsp >= uregs.xsp) {
if (debug)
VG_(printf) (" FF end of stack old_xsp %p >= xsp %p\n",
(void*)old_xsp, (void*)uregs.xsp);
break;
}
}
goto unwind_done;
} else {
Expand Down

0 comments on commit bd90e82

Please sign in to comment.