[v2,3/3] btrace, frame: fix crash in get_frame_type
Commit Message
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Thursday, February 11, 2016 2:39 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
> > 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?
_dl_runtime_resolve would need to tail-call _dl_fixup. I don't think it can since
it needs to (tail-)call the resolved function after _dl_fixup returns. This should
be safe, as well, then.
> 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.
You think we should add the check nevertheless in gdb/glibc-tdep.c?
The others I won't be able to test. I could do the changes and rely on buildbot
to flag issues. If we really want to change them.
I put the "info frame" changes into a separate patch and a test into the
tailcall-only.exp test. Here are the stack.c changes:
commit 0dc54d83f375e8901061244807e078f281fca070
Author: Markus Metzger <markus.t.metzger@intel.com>
Date: Thu Feb 11 11:07:09 2016 +0100
stack: check frame_unwind_caller_id
Callers of frame_unwind_caller_* functions are supposed to check
frame_unwind_caller_id.
Add such a check to frame_info and treat an invalid caller ID as if the caller
PC were not available.
2016-02-11 Markus Metzger <markus.t.metzger@intel.com>
gdb/
* stack.c (frame_info): Check frame_unwind_caller_id.
Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Comments
On 02/11/2016 03:42 PM, Metzger, Markus T wrote:
>> 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.
>
> You think we should add the check nevertheless in gdb/glibc-tdep.c?
Let's leave it be. If may be that the use case that trips on it would
be best fixed differently.
>
> The others I won't be able to test. I could do the changes and rely on buildbot
> to flag issues. If we really want to change them.
I don't think we should change the others without evidence that they'd need it.
If we do trip on problems on those, I think the fix should be elsewhere.
I'd still early in the release cycle, let's wait and see.
>
> commit 0dc54d83f375e8901061244807e078f281fca070
> Author: Markus Metzger <markus.t.metzger@intel.com>
> Date: Thu Feb 11 11:07:09 2016 +0100
>
> stack: check frame_unwind_caller_id
>
> Callers of frame_unwind_caller_* functions are supposed to check
> frame_unwind_caller_id.
>
> Add such a check to frame_info and treat an invalid caller ID as if the caller
> PC were not available.
>
> 2016-02-11 Markus Metzger <markus.t.metzger@intel.com>
>
> gdb/
> * stack.c (frame_info): Check frame_unwind_caller_id.
>
OK.
Thanks,
Pedro Alves
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, February 11, 2016 4:58 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
> >> 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.
> >
> > You think we should add the check nevertheless in gdb/glibc-tdep.c?
>
> Let's leave it be. If may be that the use case that trips on it would be best
> fixed differently.
>
> >
> > The others I won't be able to test. I could do the changes and rely
> > on buildbot to flag issues. If we really want to change them.
>
> I don't think we should change the others without evidence that they'd need
> it.
> If we do trip on problems on those, I think the fix should be elsewhere.
>
> I'd still early in the release cycle, let's wait and see.
OK. I'll send the complete series again.
Pathes 1 and 2 are unchanged and you've seen almost everything of patch 3,
already. Actually, patch 2 has been replaced by a completely unrelated patch,
but you've OK'ed it already in your last reply.
Thanks a lot for your review and for pointing out issues!
Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
@@ -1509,27 +1509,32 @@ frame_info (char *addr_exp, int from_tty)
wrap_here (" ");
printf_filtered ("saved %s = ", pc_regname);
- TRY
- {
- caller_pc = frame_unwind_caller_pc (fi);
- caller_pc_p = 1;
- }
- CATCH (ex, RETURN_MASK_ERROR)
+ if (!frame_id_p (frame_unwind_caller_id (fi)))
+ val_print_unavailable (gdb_stdout);
+ else
{
- switch (ex.error)
+ TRY
{
- case NOT_AVAILABLE_ERROR:
- val_print_unavailable (gdb_stdout);
- break;
- case OPTIMIZED_OUT_ERROR:
- val_print_not_saved (gdb_stdout);
- break;
- default:
- fprintf_filtered (gdb_stdout, _("<error: %s>"), ex.message);
- break;
+ caller_pc = frame_unwind_caller_pc (fi);
+ caller_pc_p = 1;
}
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ switch (ex.error)
+ {
+ case NOT_AVAILABLE_ERROR:
+ val_print_unavailable (gdb_stdout);
+ break;
+ case OPTIMIZED_OUT_ERROR:
+ val_print_not_saved (gdb_stdout);
+ break;
+ default:
+ fprintf_filtered (gdb_stdout, _("<error: %s>"), ex.message);
+ break;
+ }
+ }
+ END_CATCH
}
- END_CATCH
if (caller_pc_p)
fputs_filtered (paddress (gdbarch, caller_pc), gdb_stdout);