|
From: Nicholas N. <n.n...@gm...> - 2009-02-04 06:46:53
|
Hi,
The main bit of x86 stack-unwinding code is as follows:
/* On x86, first try the old-fashioned method of following the
%ebp-chain. Code which doesn't use this (that is, compiled
with -fomit-frame-pointer) is not ABI compliant and so
relatively rare. Besides, trying the CFI first almost always
fails, and is expensive. */
/* Deal with frames resulting from functions which begin "pushl%
ebp ; movl %esp, %ebp" which is the ABI-mandated preamble. */
if (fp_min <= fp && fp <= fp_max
- 1 * sizeof(UWord)/*see comment below*/) {
/* fp looks sane, so use it. */
ip = (((UWord*)fp)[1]);
sp = fp + sizeof(Addr) /*saved %ebp*/
+ sizeof(Addr) /*ra*/;
fp = (((UWord*)fp)[0]);
if (sps) sps[i] = sp;
if (fps) fps[i] = fp;
ips[i++] = ip - 1; /* -1: refer to calling insn, not the RA */
VG_(printf)(" %#lx\n", ip);
if (debug)
VG_(printf)(" ipsF[%d]=0x%08lx\n", i-1, ips[i-1]);
ip = ip - 1; /* as per comment at the head of this loop */
continue;
}
I just hit an interesting case on Darwin where the last ips[] entry
written is 0. I.e. when assigned, ip-1 was 0, ie. ip was 1.
This led me to some confusion because VG_(apply_StackTrace), which is
used by VG_(pp_StackTrace), stops if it hits an ip of zero, even if
its before n_ips has been reached. So when I was printing such a
stack the last entry was not shown.
So it seems we're currently using two distinct methods for indicating
the end of a stack trace. First, there's n_ips, and second, there's
zero entries. I think we should probably not treat zero entries
specially, and just rely on n_ips. If we do want to try to filter
out zero addresses, I think it should be done in
VG_(get_StackTrace_wrk) and then n_ips adjusted accordingly, rather
than doing such filtering subsequently.
Thoughts?
N
|
|
From: Julian S. <js...@ac...> - 2009-02-05 00:16:14
|
On Wednesday 04 February 2009, Nicholas Nethercote wrote:
> So it seems we're currently using two distinct methods for indicating
> the end of a stack trace. First, there's n_ips, and second, there's
> zero entries. I think we should probably not treat zero entries
> specially, and just rely on n_ips. If we do want to try to filter
> out zero addresses, I think it should be done in
> VG_(get_StackTrace_wrk) and then n_ips adjusted accordingly, rather
> than doing such filtering subsequently.
>
> Thoughts?
[summary of the following: I agree, but perhaps not trivial to fix
cleanly]
Yes. I suspect you of being correct :-) I believe the semantics of
this stuff has never really been sorted out properly and perhaps now
is the time.
AFAIR the stop-at-zero thing is because thread stacks (possibly including
the main stack, on ppc-linux) are actually terminated with a zero word
in the IP chain.
At the moment the basic output of VG_(get_StackTrace_wrk) is an array
of Addrs, which in pub_tool_stacktrace.h is typedefd to "StackTrace".
One obvious thing to do would be to turn StackTrace into a struct thusly:
typedef
struct { UInt n_ips; /* 1 <= .n_ips <= VG_DEEPEST_STACKTRACE */
Addr ips[VG_DEEPEST_BACKTRACE] } StackTrace;
so that at least StackTraces are self-describing w.r.t their length.
But I think that might give performance/space problems, because there
are places -- at least in Helgrind's conflicting-access machinery --
where the number of frames stored, and hence the length of the array,
is much less than VG_DEEPEST_BACKTRACE. Have a look at the defn of
"struct _RCEC".
One way round that is to use the old variable-length struct trick:
typedef
struct { UInt n_ips; /* 1 <= .n_ips <= VG_DEEPEST_STACKTRACE */
Addr ips[0] } StackTrace;
which I think might even be legitimate C nowadays. But it then gives
a problem incorporating a StackTrace into a struct _ExeContext. Or
perhaps not; it appears struct _ExeContext already uses the variable
length array trick anyway:
struct _ExeContext {
struct _ExeContext* chain;
/* A 32-bit unsigned integer that uniquely identifies this
ExeContext. Memcheck uses these for origin tracking. Values
must be nonzero (else Memcheck's origin tracking is hosed), must
be a multiple of four, and must be unique. Hence they start at
4. */
UInt ecu;
/* Variable-length array. The size is 'n_ips'; at
least 1, at most VG_DEEPEST_BACKTRACE. [0] is the current IP,
[1] is its caller, [2] is the caller of [1], etc. */
UInt n_ips;
Addr ips[0];
};
so we would just replace the last two fields with a StaclTrace.
Anybody know what the Rules of the Game are w.r.t. variable length
arrays at the end of structs? In particular, if a variable length
struct (StackTrace) is the last field of another struct (ExeContext),
does the ExeContext itself become a variable length struct?
J
|
|
From: Julian S. <js...@ac...> - 2009-02-05 09:00:47
|
On further consideration of this, a couple of comments:
1. Perhaps creating a new struct StackTrace is too much trouble.
2. At the moment the machinery operates by passing around
(ips, n_ips) pairs. Won't the proposed change cause
confusion about the meaning of n_ips? Because then there
are two lengths to record: the size of the ips array, and
the number of actually used entries in it.
So maybe it's then relevant to consider a representation
(ips, ips_size, ips_used), where 1 <= ips_size <= VG_DEEPEST_STACKTRACE
and 1 <= ips_used <= ips_size ?
J
On Wednesday 04 February 2009, Julian Seward wrote:
> On Wednesday 04 February 2009, Nicholas Nethercote wrote:
> > So it seems we're currently using two distinct methods for indicating
> > the end of a stack trace. First, there's n_ips, and second, there's
> > zero entries. I think we should probably not treat zero entries
> > specially, and just rely on n_ips. If we do want to try to filter
> > out zero addresses, I think it should be done in
> > VG_(get_StackTrace_wrk) and then n_ips adjusted accordingly, rather
> > than doing such filtering subsequently.
> >
> > Thoughts?
>
> [summary of the following: I agree, but perhaps not trivial to fix
> cleanly]
>
> Yes. I suspect you of being correct :-) I believe the semantics of
> this stuff has never really been sorted out properly and perhaps now
> is the time.
>
> AFAIR the stop-at-zero thing is because thread stacks (possibly including
> the main stack, on ppc-linux) are actually terminated with a zero word
> in the IP chain.
>
> At the moment the basic output of VG_(get_StackTrace_wrk) is an array
> of Addrs, which in pub_tool_stacktrace.h is typedefd to "StackTrace".
>
> One obvious thing to do would be to turn StackTrace into a struct thusly:
>
> typedef
> struct { UInt n_ips; /* 1 <= .n_ips <= VG_DEEPEST_STACKTRACE */
> Addr ips[VG_DEEPEST_BACKTRACE] } StackTrace;
>
> so that at least StackTraces are self-describing w.r.t their length.
>
> But I think that might give performance/space problems, because there
> are places -- at least in Helgrind's conflicting-access machinery --
> where the number of frames stored, and hence the length of the array,
> is much less than VG_DEEPEST_BACKTRACE. Have a look at the defn of
> "struct _RCEC".
>
> One way round that is to use the old variable-length struct trick:
>
> typedef
> struct { UInt n_ips; /* 1 <= .n_ips <= VG_DEEPEST_STACKTRACE */
> Addr ips[0] } StackTrace;
>
> which I think might even be legitimate C nowadays. But it then gives
> a problem incorporating a StackTrace into a struct _ExeContext. Or
> perhaps not; it appears struct _ExeContext already uses the variable
> length array trick anyway:
>
> struct _ExeContext {
> struct _ExeContext* chain;
> /* A 32-bit unsigned integer that uniquely identifies this
> ExeContext. Memcheck uses these for origin tracking. Values
> must be nonzero (else Memcheck's origin tracking is hosed), must
> be a multiple of four, and must be unique. Hence they start at
> 4. */
> UInt ecu;
> /* Variable-length array. The size is 'n_ips'; at
> least 1, at most VG_DEEPEST_BACKTRACE. [0] is the current IP,
> [1] is its caller, [2] is the caller of [1], etc. */
> UInt n_ips;
> Addr ips[0];
> };
>
> so we would just replace the last two fields with a StaclTrace.
>
> Anybody know what the Rules of the Game are w.r.t. variable length
> arrays at the end of structs? In particular, if a variable length
> struct (StackTrace) is the last field of another struct (ExeContext),
> does the ExeContext itself become a variable length struct?
>
> J
>
> ---------------------------------------------------------------------------
>--- Create and Deploy Rich Internet Apps outside the browser with
> Adobe(R)AIR(TM) software. With Adobe AIR, Ajax developers can use existing
> skills and code to build responsive, highly engaging applications that
> combine the power of local resources and data with the reach of the web.
> Download the Adobe AIR SDK and Ajax docs to start building applications
> today-http://p.sf.net/sfu/adobe-com
> _______________________________________________
> Valgrind-developers mailing list
> Val...@li...
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
|
|
From: Nicholas N. <n.n...@gm...> - 2009-02-05 21:29:22
|
On Thu, Feb 5, 2009 at 8:00 PM, Julian Seward <js...@ac...> wrote: > > On further consideration of this, a couple of comments: > > 1. Perhaps creating a new struct StackTrace is too much trouble. Possibly... if it's variable-sized then you can't stack-allocate it (can you?) and that would inconvenience many existing uses. > 2. At the moment the machinery operates by passing around > (ips, n_ips) pairs. Won't the proposed change cause > confusion about the meaning of n_ips? Because then there > are two lengths to record: the size of the ips array, and > the number of actually used entries in it. > > So maybe it's then relevant to consider a representation > (ips, ips_size, ips_used), where 1 <= ips_size <= VG_DEEPEST_STACKTRACE > and 1 <= ips_used <= ips_size ? Yep. I'll likely do something like this when I get to looking at it. N |
|
From: Julian S. <js...@ac...> - 2009-02-05 23:12:49
|
> > On further consideration of this, a couple of comments: > > > > 1. Perhaps creating a new struct StackTrace is too much trouble. > > Possibly... if it's variable-sized then you can't stack-allocate it > (can you?) I wouldn't have thought so, no. > > So maybe it's then relevant to consider a representation > > (ips, ips_size, ips_used), where 1 <= ips_size <= VG_DEEPEST_STACKTRACE > > and 1 <= ips_used <= ips_size ? > > Yep. I'll likely do something like this when I get to looking at it. Good. J |