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

[Pedro Alves <palves@redhat.com>]

On 02/11/2016 09:51 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Wednesday, February 10, 2016 4:34 PM
>> To: Metzger, Markus T <markus.t.metzger@intel.com>
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type
>>
>> 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:
> 
> That's the sentinel frame.  I forgot to "up".  Now it crashes;-)
> 
> There are other cases where frame_unwind_caller_xxx callers don't check
> frame_unwind_caller_id:
> 
> 	gdb/mips-linux-tdep.c
> 	gdb/glibc-tdep.c
> 	gdb/obsd-tdep.c
> 	gdb/tic6x-linux-tdep.c
> 	gdb/sol2-tdep.c
> 	gdb/nios2-linux-tdep.c
> 
> They're used for skipping syscalls and ld.so.
> 
> The latter should be called via gdbarch_skip_solib_resolver (gdbarch, stop_pc)
> from infrun.c.
> 
> Who is supposed to do the check in those cases?  Maybe they are already OK?

In the syscall cases, we're trying to determine the next PC where to place a
breakpoint, in order to do a software single-step.  If we don't know where the
caller is, we can't single-step, so we should probably error out.  OTOH, if the
target_ops is record-like and we're single-stepping through the trace log,
we shouldn't be trying to use software single-step at all.  So I think those
are probably OK.

In the glibc_skip_solib_resolver case -- in theory, I guess it would be
possible to construct a branch trace that records a tailcall to _dl_fixup,
and that doesn't have any frame above that one?

If we don't know where the caller is, we can't skip the resolver
in one go, so best to do is probably to return 0, and let infrun's
stepping logic continue single-stepping.

Thanks,
Pedro Alves