Re: [patch v8 05/24] frame: artificial frame id's

[Pedro Alves <palves@redhat.com>]

On 12/13/2013 08:04 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
>> Sent: Thursday, December 12, 2013 8:56 PM
> 
> Thanks for your feedback, Pedro.
> 
> Jan already answered some of your questions.
> [...]
> 
>>> Looks like frame_ id_eq (null_frame_id, null_frame_id) now
>>> returns true, while it returns false before.  If you discussed
>>> all this and came to the conclusion it's OK, please document
>>> it in the commit log.  In any case (I'm not sure offhand if
>>> that's OK), the NaN comment above is no longer correct,
>>> and neither is this one:
>>
>> That was not needed / intentional so it would be better to keep the original
>> behavior.
> 
> Would it be OK to have no frame be equal to null_frame_id?

That looks messy and piling hacks.  Seem to me the NaN test
could be replaced by frame_id_p checks instead.  But,
please see (git 005ca36a) and the history behind this code:

https://sourceware.org/ml/gdb-patches/2009-09/msg00271.html
https://sourceware.org/ml/gdb-patches/2009-08/msg00524.html

The exiting code&comments ...

int
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.  */
    eq = 1;



and this in infrun.c:

     NOTE: frame_id_eq will never report two invalid frame IDs as
     being equal, so to get into this block, both the current and
     previous frame must have valid frame IDs.  */
  /* The outer_frame_id check is a heuristic to detect stepping
     through startup code.  If we step over an instruction which
     sets the stack pointer from an invalid value to a valid value,
     we may detect that as a subroutine call from the mythical
     "outermost" function.  This could be fixed by marking
     outermost frames as !stack_p,code_p,special_p.  Then the
     initial outermost frame, before sp was valid, would
     have code_addr == &_start.  See the comment in frame_id_eq
     for more.  */
  if (!frame_id_eq (get_stack_frame_id (frame),
		    ecs->event_thread->control.step_stack_frame_id)
      && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
		       ecs->event_thread->control.step_stack_frame_id)
	  && (!frame_id_eq (ecs->event_thread->control.step_stack_frame_id,
			    outer_frame_id)
	      || step_start_function != find_pc_function (stop_pc))))
    {


... seems to me was headed in the direction of having different
outermost frame ids for different functions, IOW, frame ids
with code_p, but different code_addr's, and still
with {!stack_addr_p,special_addr_p}.  The code above seems to want
that those would still compare equal, as they're all outermost.
I don't think we actually do create such ids anywhere, though,
and I'm not sure whether ignoring the code_addr like that is
really the best way to go.

In any case, I think we do have several distinct cases:

 #1 - we're in the outermost frame / entry pointer, haven't
   setup the stack yet.
 #2 - we're in the outermost frame (identified by debug info or ABI,
   e.g., frame pointer == 0), we've setup the stack.
 #3 - we're in the outermost frame (identified by debug info),
     but stack pointer/address is unavailable (haven't collected register).
 #4 - non outermost frame, there a stack, and it is known.
 #5 - non outermost frame, there a stack, and it is unavailable.

And I don't think it's a good idea to make fixing these outermost
issues harder, by introducing reuse of the same frame id markers
for different things.

I've slowly been moving the code in the direction
of identifying the outermost frame not by id, but by having
the unwinder identify that with:

 this_frame->unwind->stop_reason () == UNWIND_OUTERMOST.

So with that aside, we still have:

 - invalid stack addr
 - valid stack addr
 - valid, but unavailable stack addr

Take a look at this patch:

  https://sourceware.org/ml/gdb-patches/2013-04/msg00541.html

It seems to me your frames are exactly like those, though
it sounds like you may have more than one frame with stack
unavailable, and with the same code address, but that should
be identified as different frames (please confirm).  In that
case, it seems like you could use the artificial depth to
distinguish them.

-- 
Pedro Alves