|
From: <sv...@va...> - 2008-12-12 13:23:10
|
Author: sewardj
Date: 2008-12-12 13:23:03 +0000 (Fri, 12 Dec 2008)
New Revision: 8818
Log:
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.
Modified:
trunk/coregrind/m_debuginfo/debuginfo.c
trunk/coregrind/m_stacktrace.c
trunk/include/pub_tool_execontext.h
trunk/include/pub_tool_stacktrace.h
Modified: trunk/coregrind/m_debuginfo/debuginfo.c
===================================================================
--- trunk/coregrind/m_debuginfo/debuginfo.c 2008-12-12 08:08:58 UTC (rev 8817)
+++ trunk/coregrind/m_debuginfo/debuginfo.c 2008-12-12 13:23:03 UTC (rev 8818)
@@ -2404,29 +2404,15 @@
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 @@
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;
}
Modified: trunk/coregrind/m_stacktrace.c
===================================================================
--- trunk/coregrind/m_stacktrace.c 2008-12-12 08:08:58 UTC (rev 8817)
+++ trunk/coregrind/m_stacktrace.c 2008-12-12 13:23:03 UTC (rev 8818)
@@ -140,11 +140,6 @@
* 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 @@
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 @@
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 @@
* 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 @@
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 @@
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 @@
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 @@
{
# 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 @@
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 @@
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
Modified: trunk/include/pub_tool_execontext.h
===================================================================
--- trunk/include/pub_tool_execontext.h 2008-12-12 08:08:58 UTC (rev 8817)
+++ trunk/include/pub_tool_execontext.h 2008-12-12 13:23:03 UTC (rev 8818)
@@ -54,6 +54,9 @@
// 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 );
Modified: trunk/include/pub_tool_stacktrace.h
===================================================================
--- trunk/include/pub_tool_stacktrace.h 2008-12-12 08:08:58 UTC (rev 8817)
+++ trunk/include/pub_tool_stacktrace.h 2008-12-12 13:23:03 UTC (rev 8818)
@@ -41,6 +41,18 @@
// 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.
|