From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp00.avonet.cz (smtp00.avonet.cz [217.112.162.55]) by sourceware.org (Postfix) with ESMTP id 9E7913858C1F for ; Tue, 22 Nov 2022 06:48:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9E7913858C1F Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=fbl.cz Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=fbl.cz Received: from ktus.lan (217-115-245-101.cust.avonet.cz [217.115.245.101]) by smtp00.avonet.cz (Postfix) with ESMTP id 4NGZb85JPVz1xqW for ; Tue, 22 Nov 2022 07:48:20 +0100 (CET) Received: by ktus.lan (Postfix, from userid 209) id A163F31306A; Tue, 22 Nov 2022 07:48:20 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-Spam-Level: X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,NICE_REPLY_A,SPF_HELO_NONE,TXREP,T_SPF_PERMERROR autolearn=ham autolearn_force=no version=3.4.6 Received: from [192.168.33.9] (217-115-245-101.cust.avonet.cz [217.115.245.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: vanekt) by ktus.lan (Postfix) with ESMTPSA id 2863B313066; Tue, 22 Nov 2022 07:48:14 +0100 (CET) Message-ID: <38d21d64-b116-be98-c085-084996f97140@fbl.cz> Date: Tue, 22 Nov 2022 07:48:16 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [PATCH v2] gdb: Modify until_break_command to act correctly on SIGTRAMP_FRAME Content-Language: en-GB From: Tomas Vanek To: Luis Machado , gdb-patches@sourceware.org References: <1666353538-15846-1-git-send-email-vanekt@fbl.cz> <8c72008e-6151-846d-77dd-7da642b1489f@arm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit List-Id: 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 >>> --- >>>   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 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? Or do we need it reviewed by somebody else? Tomas