From: Julian Seward Date: Fri, 12 Dec 2008 13:23:03 +0000 (+0000) Subject: This commit subtly changes the meaning of the values obtained via the X-Git-Tag: svn/VALGRIND_3_4_0~79 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ba2ece03b8f37200ccf28fca416097a57fa10014;p=thirdparty%2Fvalgrind.git This commit subtly changes the meaning of the values obtained via the stack unwind mechanism (the function VG_(record_ExeContext) et al), clears up some associated kludges, and makes suppression matching work more reliably. Prior to this commit, a stack snapshot contained, at [0], the IP of the relevant thread, and at all positions [1] and above, the return addresses for the open calls. When showing a snapshot to the user (in VG_(apply_StackTrace)), and searching the stack for stack blocks (in VG_(get_data_description)), 1 is subtracted from positions [1] and above, so as to move these return addresses back to the last byte of the calling instruction. This subtraction is also done even in VG_(get_StackTrace_wrk) itself, in order to make the stack unwinding work at all. It turns out that suppression-vs-function-name matching requires the same hack, and sometimes failed to match suppressions that should match, because of this self-same problem. So the commit changes the stack unwinder itself, so that entries [1] and above point to the last byte of the call instruction, rather than the return address. The associated kludges in VG_(apply_StackTrace) and VG_(get_StackTrace_wrk) are removed, and suppression matching is observed to work in a case where it failed before. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@8818 --- diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c index 1bebc56f8e..f244f0255d 100644 --- a/coregrind/m_debuginfo/debuginfo.c +++ b/coregrind/m_debuginfo/debuginfo.c @@ -2404,29 +2404,15 @@ Bool VG_(get_data_description)( /*OUT*/Char* dname1, frames, and for each frame, consider the local variables. */ n_frames = VG_(get_StackTrace)( tid, ips, N_FRAMES, sps, fps, 0/*first_ip_delta*/ ); - /* Re ip_delta in the next loop: There's a subtlety in the meaning - of the IP values in a stack obtained from VG_(get_StackTrace). - The innermost value really is simply the thread's program - counter at the time the snapshot was taken. However, all the - other values are actually return addresses, and so point just - after the call instructions. Hence they notionally reflect not - what the program counters were at the time those calls were - made, but what they will be when those calls return. This can - be of significance should an address range happen to end at the - end of a call instruction -- we may ignore the range when in - fact it should be considered. Hence, back up the IPs by 1 for - all non-innermost IPs. Note that VG_(get_StackTrace_wrk) itself - has to use the same trick in order to use CFI data to unwind the - stack (as documented therein in comments). */ + /* As a result of KLUDGE above, starting the loop at j = 0 duplicates examination of the top frame and so isn't necessary. Oh well. */ vg_assert(n_frames >= 0 && n_frames <= N_FRAMES); for (j = 0; j < n_frames; j++) { - Word ip_delta = j == 0 ? 0 : 1; if (consider_vars_in_frame( dname1, dname2, n_dname, data_addr, - ips[j] - ip_delta, + ips[j], sps[j], fps[j], tid, j )) { dname1[n_dname-1] = dname2[n_dname-1] = 0; return True; @@ -2445,14 +2431,15 @@ Bool VG_(get_data_description)( /*OUT*/Char* dname1, amd64), the variable's location list does claim it exists starting at the first byte of the first instruction after the call instruction. So, call consider_vars_in_frame a second - time, but this time don't subtract 1 from the IP. GDB - handles this example with no difficulty, which leads me to - believe that either (1) I misunderstood something, or (2) GDB - has an equivalent kludge. */ - if (consider_vars_in_frame( dname1, dname2, n_dname, - data_addr, - ips[j], - sps[j], fps[j], tid, j )) { + time, but this time add 1 to the IP. GDB handles this + example with no difficulty, which leads me to believe that + either (1) I misunderstood something, or (2) GDB has an + equivalent kludge. */ + if (j > 0 /* this is a non-innermost frame */ + && consider_vars_in_frame( dname1, dname2, n_dname, + data_addr, + ips[j] + 1, + sps[j], fps[j], tid, j )) { dname1[n_dname-1] = dname2[n_dname-1] = 0; return True; } diff --git a/coregrind/m_stacktrace.c b/coregrind/m_stacktrace.c index f835dcaf8f..bd1e42a6cf 100644 --- a/coregrind/m_stacktrace.c +++ b/coregrind/m_stacktrace.c @@ -140,11 +140,6 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, * This most frequently happens at the end of a function when * a tail call occurs and we wind up using the CFI info for the * next function which is completely wrong. - * - * Note that VG_(get_data_description) (in m_debuginfo) has to take - * this same problem into account when unwinding the stack to - * examine local variable descriptions (as documented therein in - * comments). */ while (True) { @@ -170,10 +165,10 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, fp = (((UWord*)fp)[0]); if (sps) sps[i] = sp; if (fps) fps[i] = fp; - ips[i++] = ip; + ips[i++] = ip - 1; /* -1: refer to calling insn, not the RA */ if (debug) VG_(printf)(" ipsF[%d]=0x%08lx\n", i-1, ips[i-1]); - ip = ip - 1; + ip = ip - 1; /* as per comment at the head of this loop */ continue; } @@ -182,10 +177,10 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, if ( VG_(use_CF_info)( &ip, &sp, &fp, fp_min, fp_max ) ) { if (sps) sps[i] = sp; if (fps) fps[i] = fp; - ips[i++] = ip; + ips[i++] = ip - 1; /* -1: refer to calling insn, not the RA */ if (debug) VG_(printf)(" ipsC[%d]=0x%08lx\n", i-1, ips[i-1]); - ip = ip - 1; + ip = ip - 1; /* as per comment at the head of this loop */ continue; } @@ -218,11 +213,6 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, * This most frequently happens at the end of a function when * a tail call occurs and we wind up using the CFI info for the * next function which is completely wrong. - * - * Note that VG_(get_data_description) (in m_debuginfo) has to take - * this same problem into account when unwinding the stack to - * examine local variable descriptions (as documented therein in - * comments). */ while (True) { @@ -237,10 +227,10 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, if ( VG_(use_CF_info)( &ip, &sp, &fp, fp_min, fp_max ) ) { if (sps) sps[i] = sp; if (fps) fps[i] = fp; - ips[i++] = ip; + ips[i++] = ip - 1; /* -1: refer to calling insn, not the RA */ if (debug) VG_(printf)(" ipsC[%d]=%#08lx\n", i-1, ips[i-1]); - ip = ip - 1; + ip = ip - 1; /* as per comment at the head of this loop */ continue; } @@ -264,10 +254,10 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, fp = (((UWord*)fp)[0]); if (sps) sps[i] = sp; if (fps) fps[i] = fp; - ips[i++] = ip; + ips[i++] = ip - 1; /* -1: refer to calling insn, not the RA */ if (debug) VG_(printf)(" ipsF[%d]=%#08lx\n", i-1, ips[i-1]); - ip = ip - 1; + ip = ip - 1; /* as per comment at the head of this loop */ continue; } @@ -287,10 +277,13 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, ip = ((UWord*)sp)[0]; if (sps) sps[i] = sp; if (fps) fps[i] = fp; - ips[i++] = ip; + ips[i++] = ip == 0 + ? 0 /* sp[0] == 0 ==> stuck at the bottom of a + thread stack */ + : ip - 1; /* -1: refer to calling insn, not the RA */ if (debug) VG_(printf)(" ipsH[%d]=%#08lx\n", i-1, ips[i-1]); - ip = ip - 1; + ip = ip - 1; /* as per comment at the head of this loop */ sp += 8; continue; } @@ -342,6 +335,8 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, { # define M_VG_ERRTXT 1000 UChar buf_lr[M_VG_ERRTXT], buf_ip[M_VG_ERRTXT]; + /* The following conditional looks grossly inefficient and + surely could be majorly improved, with not much effort. */ if (VG_(get_fnname_nodemangle) (lr, buf_lr, M_VG_ERRTXT)) if (VG_(get_fnname_nodemangle) (ip, buf_ip, M_VG_ERRTXT)) if (VG_(strncmp)(buf_lr, buf_ip, M_VG_ERRTXT)) @@ -410,9 +405,12 @@ UInt VG_(get_StackTrace_wrk) ( ThreadId tid_if_known, fp = (((UWord*)fp)[0]); if (sps) sps[i] = fp; /* NB. not sp */ if (fps) fps[i] = fp; - ips[i++] = ip; + ips[i++] = ip - 1; /* -1: refer to calling insn, not the RA */ if (debug) VG_(printf)(" ipsF[%d]=%#08lx\n", i-1, ips[i-1]); + ip = ip - 1; /* ip is probably dead at this point, but + play safe, a la x86/amd64 above. See + extensive comments above. */ continue; } @@ -543,8 +541,6 @@ void VG_(apply_StackTrace)( void(*action)(UInt n, Addr ip), vg_assert(n_ips > 0); do { Addr ip = ips[i]; - if (i > 0) - ip -= VG_MIN_INSTR_SZB; // point to calling line // Stop after the first appearance of "main" or one of the other names // (the appearance of which is a pretty good sign that we've gone past diff --git a/include/pub_tool_execontext.h b/include/pub_tool_execontext.h index 84dda74c04..2cbdfc49bf 100644 --- a/include/pub_tool_execontext.h +++ b/include/pub_tool_execontext.h @@ -54,6 +54,9 @@ typedef // ThreadId should be passed in by the core. The initial IP value to // use is adjusted by first_ip_delta before the stack is unwound. // A safe value to pass is zero. +// +// See comments in pub_tool_stacktrace.h for precise definition of +// the meaning of the code addresses in the returned ExeContext. extern ExeContext* VG_(record_ExeContext) ( ThreadId tid, Word first_ip_delta ); diff --git a/include/pub_tool_stacktrace.h b/include/pub_tool_stacktrace.h index 9fda8216df..47760544ff 100644 --- a/include/pub_tool_stacktrace.h +++ b/include/pub_tool_stacktrace.h @@ -41,6 +41,18 @@ typedef Addr* StackTrace; // The initial IP value to use is adjusted by first_ip_delta before // the stack is unwound. A safe value to pass is zero. // +// The specific meaning of the returned addresses is: +// +// [0] is the IP of thread 'tid' +// [1] points to the last byte of the call instruction that called [0]. +// [2] points to the last byte of the call instruction that called [1]. +// etc etc +// +// Hence ips[0 .. return_value-1] should all point to currently +// 'active' (in the sense of a stack of unfinished function calls) +// instructions. [0] points to the start of an arbitrary instruction.# +// [1 ..] point to the last byte of a chain of call instructions. +// // If sps and fps are non-NULL, the corresponding frame-pointer and // stack-pointer values for each frame are stored there.