Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type

[Pedro Alves <palves@redhat.com>]

On 02/10/2016 03:02 PM, Metzger, Markus T wrote:

> No new fails there, as well (64-bit IA).
> 
> I added a comment based on your statement that frame_unwind_caller_xxx
> callers should check frame_unwind_caller_id and assert that skip_artificial_frames
> does not return NULL.
> 
> Info frame doesn't crash.
> 
> 	(gdb) info frame
> 	Stack level 0, frame at 0x0:
> 	 rip = 0x4005b0 in bar (tailcall-only.c:29); saved rip = 0x4005c2
> 	 called by frame at 0x0
         ^^^^^^^^^^^^^^^

> 	 source 	language c.
> 	 Arglist at unknown address.
> 	 Locals at unknown address,Registers are not available in btrace record history
> 
> This is from a tailcall-only frame stack in replay mode using the tailcall-only test.
> The real caller has not been recorded.

Not sure how you got that, since "called by frame" seems to indicates that
the frame was not TAILCALL_FRAME:

  else if (get_frame_type (fi) == TAILCALL_FRAME)
    puts_filtered (" tail call frame");
  else if (get_frame_type (fi) == INLINE_FRAME)
    printf_filtered (" inlined into frame %d",
		     frame_relative_level (get_prev_frame (fi)));
  else
    {
      printf_filtered (" called by frame at ");
      fputs_filtered (paddress (gdbarch, get_frame_base (calling_frame_info)),
		      gdb_stdout);
    }


> 
> The output isn't very helpful for record btrace since we don't record register and
> memory changes.


So I'm mostly OK with the patch now, but I think you should dig a bit more
into the "info frame" output, since I think you _will_ internal error with a
TAILCALL_FRAME.

My remaining issue is now with the user-visible strings.

> @@ -985,6 +1007,10 @@ frame_pop (struct frame_info *this_frame)
>       entering THISFRAME.  */
>    prev_frame = skip_tailcall_frames (prev_frame);
>  
> +  /* We cannot pop tailcall frames.  */
> +  if (prev_frame == NULL)
> +    error (_("Cannot pop a tailcall frame."));
> +

I find this confusing, from a user perspective.  AFAIK, you can pop a tailcall
frame; what you can't do is pop when you don't know anything about the frame
that started the tail calling.  How about:

  if (prev_frame == NULL)
    error (_("Cannot return: tailcall caller frame not found."));

s/pop/return/, as "pop" is an internal implementation detail.

(I suggest also dropping the redundant comment.)


> +    {
> +      /* Ignore TAILCALL_FRAME type frames, they were executed already before
> +	 entering THISFRAME.  */
> +      frame = skip_tailcall_frames (frame);
> +
> +      if (frame == NULL)
> +	error (_("\"finish\" not meaningful for tailcall frames."));
> +

  if (frame == NULL)
    error (_("Cannot finish: tailcall caller frame not found."));


(I'd also be fine with dropping the "Cannot xxx:" part to make
the error messages the same in both cases.)


> +      finish_forward (sm, frame);
> +    }
>  }

Thanks,
Pedro Alves