public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Luis Machado <luis.machado@arm.com>
To: Tomas Vanek <vanekt@fbl.cz>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2] gdb: Modify until_break_command to act correctly on SIGTRAMP_FRAME
Date: Tue, 22 Nov 2022 07:27:05 +0000	[thread overview]
Message-ID: <0cbcd0ed-9702-2c8e-f356-5b68e24dbb0d@arm.com> (raw)
In-Reply-To: <38d21d64-b116-be98-c085-084996f97140@fbl.cz>

Hi Tomas,

On 11/22/22 06:48, Tomas Vanek wrote:
> On 27/10/2022 19:46, Tomas Vanek wrote:
>> Hi Luis,
>>
>> On 27/10/2022 12:31, Luis Machado wrote:
>>> Hi Tomas,
>>>
>>> On 10/21/22 12:58, Tomas Vanek wrote:
>>>> This patch partially depends on
>>>> gdb/arm: Terminate frame unwinding in M-profile lockup state
>>>> (without it lockup state is unwound as if it were a normal
>>>> stack frame).
>>>>
>>>> The commands 'advance' and 'until' try to set a breakpoint
>>>> on the bogus return address derived from Arm M-profile magic
>>>> address (actually EXC_RETURN or a PC value indicating lockup).
>>>>
>>>> The offending breakpoint should be set at the return address in
>>>> the caller. The magic value 0xffffffff in LR indicates
>>>> there is no caller (return to this address would lock up the CPU).
>>>>
>>>> Similar behaviour of 'advance' and 'until' is observed in
>>>> an exception handler routine. In this case LR contains e.g.
>>>> 0xfffffff1 (EXC_RETURN) and GDB tries to set a breakpoint at
>>>> 0xfffffff0. It should use a return value stacked by the exception
>>>> instead.
>>>>
>>>> Testbench setup:
>>>> STM32G474, a Cortex-M4 device. Any Cortex-M device can be used.
>>>> A test application (an ordinary blink) with a standard startup
>>>> is loaded to the device flash.
>>>>
>>>> Steps to reproduce the problem:
>>>>
>>>> start GDB server
>>>>   $ openocd -f interface/cmsis-dap.cfg -f target/stm32g4x.cfg
>>>>
>>>> start GDB in second terminal
>>>>   $ arm-none-eabi-gdb blink.elf
>>>>
>>>>   (gdb) target extended-remote localhost:3333
>>>>
>>>> Reset the device and halt it:
>>>>   (gdb) monitor reset halt
>>>>   target halted due to debug-request, current mode: Thread
>>>>   xPSR: 0x01000000 pc: 0x08000e14 msp: 0x20020000
>>>>
>>>> Step by one instruction to re-read GDB register cache:
>>>>   (gdb) stepi
>>>>
>>>> Check registers, LR should be 0xffffffff after reset:
>>>>   (gdb) info registers
>>>>   ...
>>>>   sp             0x20020000          0x20020000
>>>>   lr             0xffffffff          -1
>>>>   pc             0x8000e16           0x8000e16
>>>>   xPSR           0x1000000           16777216
>>>>   ...
>>>>
>>>>   (gdb) set debug remote
>>>>
>>>> Issue 'advance' command:
>>>>   (gdb) advance main
>>>>   [remote] Sending packet: $mfffffffe,2#fa
>>>>   [remote] Packet received: 0000
>>>>   [remote] Sending packet: $mfffffffe,2#fa
>>>>   [remote] Packet received: 0000
>>>>   [remote] Sending packet: $m8000526,2#30
>>>>   [remote] Packet received: 2046
>>>>   [remote] Sending packet: $Z1,8000526,2#7a
>>>>   [remote] Packet received: OK
>>>>   [remote] packet_ok: Packet Z1 (hardware-breakpoint) is supported
>>>>   [remote] Sending packet: $Z0,fffffffe,2#43
>>>>   [remote] Packet received: E0E
>>>>   [remote] packet_ok: Packet Z0 (software-breakpoint) is supported
>>>>   Warning:
>>>>   Cannot insert breakpoint 0.
>>>>   Cannot access memory at address 0xfffffffe
>>>>
>>>>   Command aborted.
>>>>   (gdb)
>>>>
>>>> Relevant messages from OpenOCD:
>>>>   Error: Failed to read memory at 0xfffff000
>>>>   Error: can't add breakpoint: unknown reason
>>>>
>>>> This patch adds skipping over frames that are not suitable for
>>>> guarding with a breakpoint inspired by 'finish' command.
>>>> If no suitable frame is found, a momentary breakpoint is not set.
>>>>
>>>> v2: Comment fixes, bug reference.
>>>>
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28683
>>>> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
>>>> ---
>>>>   gdb/breakpoint.c | 22 +++++++++++++++++-----
>>>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>>> index f6591d4..bb85342 100644
>>>> --- a/gdb/breakpoint.c
>>>> +++ b/gdb/breakpoint.c
>>>> @@ -10467,6 +10467,7 @@ enum async_reply_reason
>>>>   until_break_command (const char *arg, int from_tty, int anywhere)
>>>>   {
>>>>     frame_info_ptr frame;
>>>> +  frame_info_ptr caller_frame;
>>>>     struct gdbarch *frame_gdbarch;
>>>>     struct frame_id stack_frame_id;
>>>>     struct frame_id caller_frame_id;
>>>> @@ -10505,7 +10506,17 @@ enum async_reply_reason
>>>>     frame = get_selected_frame (NULL);
>>>>     frame_gdbarch = get_frame_arch (frame);
>>>>     stack_frame_id = get_stack_frame_id (frame);
>>>> -  caller_frame_id = frame_unwind_caller_id (frame);
>>>> +
>>>> +  caller_frame = get_prev_frame_always (frame);
>>>> +
>>>> +  while (caller_frame)
>>>> +    {
>>>> +      if (get_frame_type (caller_frame) != TAILCALL_FRAME
>>>> +      && gdbarch_code_of_frame_writable (get_frame_arch (caller_frame), caller_frame))
>>>> +    break;
>>>> +
>>>> +      caller_frame = get_prev_frame_always (caller_frame);
>>>> +    }
>>>>       /* Keep within the current frame, or in frames called by the current
>>>>        one.  */
>>>> @@ -10514,14 +10525,15 @@ enum async_reply_reason
>>>>       gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;
>>>>   -  if (frame_id_p (caller_frame_id))
>>>> +  if (caller_frame)
>>>>       {
>>>>         struct symtab_and_line sal2;
>>>>         struct gdbarch *caller_gdbarch;
>>>>   -      sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0);
>>>> -      sal2.pc = frame_unwind_caller_pc (frame);
>>>> -      caller_gdbarch = frame_unwind_caller_arch (frame);
>>>> +      sal2 = find_pc_line (get_frame_pc (caller_frame), 0);
>>>> +      sal2.pc = get_frame_pc (caller_frame);
>>>> +      caller_gdbarch = get_frame_arch (caller_frame);
>>>> +      caller_frame_id = get_frame_id (caller_frame);
>>>>           breakpoint_up caller_breakpoint
>>>>       = set_momentary_breakpoint (caller_gdbarch, sal2,
>>>
>>> My understanding is that these changes are meant to prevent both commands (until/advance) from
>>> attempting to place a breakpoint in a caller that doesn't really exist, right?
>>>
>> Yes.
>>
>>> The finish command, as you mentioned, seems to have a similar treatment in "skip_finish_frames".
>>>
>>> Would it be possible to factor out that code into a common function that we can call to determine
>>> if we have a valid caller whose PC we can breakpoint?
>>
>> Of course it was also my original idea.
>>
>> Unfortunately skip_finish_frames() uses skip_tailcall_frames() and skip_unwritable_frames()
>> which both call get_prev_frame(). get_prev_frame() stops if the current frame is main() or startup.
>> This is probably sufficient for the 'finish' command (until the user requests to 'finish' the main() function:
>> 'finish' refuses to do so with message "finish not meaningful in the outermost frame").
>>
>> 'advance' places one breakpoint according to the user request.
>> The second one is set as a safety measure for the case the first one is not reached.
>> It is quite common to use 'advance' in the main() function and even to execute the startup code
>> and stop at the main() begin. IMO gdb should treat main() as any other function and place the safety
>> breakpoint at its return if possible.
>>
>> That's why I use get_prev_frame_always() instead of get_prev_frame().
>> And this is also the reason why there is no simple and elegant way to factor out a common function for
>> both 'advance' and 'finish'.
>>
>> Tomas
> 
> Luis,
> 
> do you have more comments on this patch?

I think the patch makes sense to me, according to what you explained.

> Or do we need it reviewed by somebody else?

Yes, global maintainers need to approve the generic parts of patches. It would be nice to get some feedback from others.

> 
> Tomas


  reply	other threads:[~2022-11-22  7:27 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-21 11:58 Tomas Vanek
2022-10-21 11:58 ` Tomas Vanek
2022-10-27 10:31 ` Luis Machado
2022-10-27 17:46   ` Tomas Vanek
2022-11-22  6:48     ` Tomas Vanek
2022-11-22  7:27       ` Luis Machado [this message]
2022-11-28 11:48 ` [PING] " Tomas Vanek
2022-12-08  1:15   ` Luis Machado
2022-12-21  8:52     ` [PING 2] " Tomas Vanek
2023-01-10 13:19     ` [PING 3] " Tomas Vanek
2023-01-10 15:31 ` Simon Marchi
2023-01-10 16:33   ` Tomas Vanek
2023-01-10 17:48 ` Simon Marchi
2023-01-10 23:22   ` Tomas Vanek
2023-01-11  1:38     ` Simon Marchi
2023-02-02  6:38       ` Tomas Vanek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0cbcd0ed-9702-2c8e-f356-5b68e24dbb0d@arm.com \
    --to=luis.machado@arm.com \
    --cc=gdb-patches@sourceware.org \
    --cc=vanekt@fbl.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).