Re: [patch/rfc] Add a sentinel frame

[Andrew Cagney <ac131313 at redhat dot com>]

> Offhand, we do _not_ pass in the frame base - we base in the base for
> the next frame.  get_prev_frame makes the same get_frame_pc call.

Oops, yes.  Anyway.

The old code would `randomly' call either frame_saved_pc or frame_chain. 
 I mean randomly, you'd think you had it licked and then discover that 
for some edge case the two calls were reversed.

The new get_prev_frame carefully orders these calls so that the sequence:

	frame_pc_unwind()
then
	frame_id_unwind()

always occures.

> You've lost the call to inside_entry_func.  Why?  You've changed the
> inside_entry_file check to check the PC for the next frame instead of
> the forthcoming frame, which is not at all the same thing.  Why?

Dig up old notes.

This is the test you added.  It stops the unwind past main:

  if (next_frame->level >= 0
      && !backtrace_below_main
      && inside_main_func (get_frame_pc (next_frame)))
    /* Don't unwind past main(), bug always unwind the sentinel frame.
       Note, this is done _before_ the frame has been marked as
       previously unwound.  That way if the user later decides to
       allow unwinds past main(), that just happens.  */
    return NULL;

It occures first (as it should).  It occures before any 
frame_id_unwind() as needed by frame_chain_valid.  It also occures 
before the test:

  /* Only try to do the unwind once.  */
  if (next_frame->prev_p)
    return next_frame->prev;
  next_frame->prev_p = 1;

so that frame flush code was eliminated (ya!).

On the other hand, if GDB is to unwind past main (presumably, if 
s/backtrace_below_main/unwind_past_main/ is false) it does the test:

  if (inside_entry_file (get_frame_pc (next_frame)))

(note the comments about how, if this becomes optional, it should also 
be moved to before `next_frame->prev_p = 1').

Anyway, now that missing test.  frame_chain_valid() also contained:

  /* If we're inside the entry file, it isn't valid.  */
  if (inside_entry_file (frame_pc_unwind (fi)))
      return 0;

Note the frame_pc_unwind().  This test is looking one level along the 
stack frame to determine if it should unwind to that level.  That is, 
when FI->prev->pc is in the entry_file, don't unwind to FI->prev.  The 
problem is, FI->prev->pc is in entry_file when FI->pc is in main.

Even when unwind-past-main is disabled, GDB refuses to unwind past main! 
 Consequently, on the branch, I dropped the test.

(It also unwinds the PC when we're probably not ready).

--

As things progress, and more targets switch to the new code, the tests 
in get_prev_frame will most likely evolve.  However, I don't know that 
we want to be adding tests without hard evidence that they are needed :-/

Having said that, sanity checks that the frame didn't go backwards:
	!frame_id_inner (frame_id, get_frame_id (next_frame))?
and that they changed:
	!frame_id_eq (frame_id, get_frame_id (next_frame))?
probably wouldn't hurt.

> You've lost the hook for an architecture-specific FRAME_CHAIN_VALID_P. 
> I've asked you about this before and I still don't understand where you
> want that logic to go.  The impression I've gotten is that you want it
> to vanish, and that doesn't make any sense.

If the frame isn't valid, the per architecture frame_id_unwind() returns 
a null frame ID (tested using frame_id_p()).  No need for that redundant 
test.

Andrew