RE: [patch v6 18/21] record-btrace: extend unwinder

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Jan Kratochvil
> Sent: Sunday, October 06, 2013 9:50 PM

Hello Jan,

Thanks for your feedback.  I've been busy otherwise the last few weeks;
now I'm switching back to the btrace series.


> > +  while (bfun->segment.prev != NULL)
> > +    bfun = bfun->segment.prev;
> > +
> > +  stack = 0;
> 
> As discussed elsewhere I do not find correct to set stack_p = 1 && stack = 0.
> I think frame_id_p() can return true even if special_p == 1 but stack_p == 0.
> You sure need some new function similar to frame_id_build_special().

Besides frame_id_p I also had to change frame_id_eq.  Here's how it look
like.  Are you OK with such changes?  Should I put the frame.[hc] changes into
a separate patch, as well?

I had to make the first test in frame_id_eq stricter than it was since
otherwise all artificial frames would have been equal to each other.
I ran the full test suite without regressions (native on 64bit IA FC19), but I'm
Not exactly sure what has been tested.  The report lists 34 untested and 133
unsupported for both runs.


diff --git a/gdb/frame.c b/gdb/frame.c
index 3157167..e8dd029 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -491,6 +491,19 @@ frame_id_build_wild (CORE_ADDR stack_addr)
   return id;
 }
 
+struct frame_id
+frame_id_build_artificial (CORE_ADDR code_addr,
+			   CORE_ADDR special_addr)
+{
+  struct frame_id id = null_frame_id;
+
+  id.code_addr = code_addr;
+  id.code_addr_p = 1;
+  id.special_addr = special_addr;
+  id.special_addr_p = 1;
+  return id;
+}
+
 int
 frame_id_p (struct frame_id l)
 {
@@ -501,6 +514,9 @@ frame_id_p (struct frame_id l)
   /* outer_frame_id is also valid.  */
   if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
     p = 1;
+  /* An artificial frame is also valid.  */
+  if (!p && l.code_addr_p && l.special_addr_p)
+    p = 1;
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=");
@@ -524,13 +540,11 @@ frame_id_eq (struct frame_id l, struct frame_id r)
 {
   int eq;
 
-  if (!l.stack_addr_p && l.special_addr_p
-      && !r.stack_addr_p && r.special_addr_p)
-    /* The outermost frame marker is equal to itself.  This is the
-       dodgy thing about outer_frame_id, since between execution steps
-       we might step into another function - from which we can't
-       unwind either.  More thought required to get rid of
-       outer_frame_id.  */
+  if (memcmp (&l, &r, sizeof (l)) == 0)
+    /* Every frame is equal to itself.
+       This is the dodgy thing about outer_frame_id, since between execution
+       steps we might step into another function - from which we can't unwind
+       either.  More thought required to get rid of outer_frame_id.  */
     eq = 1;
   else if (!l.stack_addr_p || !r.stack_addr_p)
     /* Like a NaN, if either ID is invalid, the result is false.
diff --git a/gdb/frame.h b/gdb/frame.h
index 7109dbc..9fd278c 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -174,6 +174,12 @@ extern struct frame_id frame_id_build_special (CORE_ADDR stack_addr,
    as the special identifier address are set to indicate wild cards.  */
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
+/* Construct an artificial frame ID.  The first parameter is the frame's
+   constant code address (typically the entry point), and the second the
+   frame's special identifier address.  */
+extern struct frame_id frame_id_build_artificial (CORE_ADDR code_addr,
+						  CORE_ADDR special_addr);
+
 /* Returns non-zero when L is a valid frame (a valid frame has a
    non-zero .base).  The outermost frame is valid even without an
    ID.  */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 02b5e3d..63f09c6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1036,7 +1036,7 @@ record_btrace_frame_this_id (struct frame_info *this_frame, void **this_cache,
 {
   const struct btrace_frame_cache *cache;
   const struct btrace_function *bfun;
-  CORE_ADDR stack, code, special;
+  CORE_ADDR code, special;
 
   cache = *this_cache;
 
@@ -1046,11 +1046,10 @@ record_btrace_frame_this_id (struct frame_info *this_frame, void **this_cache,
   while (bfun->segment.prev != NULL)
     bfun = bfun->segment.prev;
 
-  stack = 0;
   code = get_frame_func (this_frame);
   special = bfun->number;
 
-  *this_id = frame_id_build_special (stack, code, special);
+  *this_id = frame_id_build_artificial (code, special);
 
   DEBUG ("[frame] %s id: (!stack, pc=%s, special=%s)",
 	 btrace_get_bfun_name (cache->bfun),


> > +  code = get_frame_func (this_frame);
> > +  special = (CORE_ADDR) bfun;
> 
> This is not safe, GDB host may be 64-bit and target 32-bit and in such case
> without --enable-64-bit-bfd there will be sizeof (CORE_ADDR) == 4,
> therefore different BFUNs may get the same SPECIAL.
> bfun->insn_offset or bfun->insn_offset should serve better I think.

I changed this to use bfun->number, instead.


> Also there could be a comment like (it still applies if one uses any of bfun,
> bfun->insn_offset or bfun->insn_offset):
>   /* The btrace_function structures can be rebuilt but only after inferior has
>      run.  In such case all btrace frame have been deleted and there remain no
>      stale uses of BFUN addresses.  */

Doesn't gdb keep frame id's alive during stepping?  I thought they were used
to detect steps into subroutines.

We could end up with the same frame id but a different frame if we
recomputed the entire trace (i.e. the trace buffer overflows and we can't
stitch traces) and we happened to have the same function at the same
position in the trace.

But this might also happen for all other frame id's.


Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052