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 158583865C21 for ; Thu, 27 Oct 2022 17:46:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 158583865C21 Authentication-Results: sourceware.org; dmarc=none (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 4MytQZ58RVz1xsv for ; Thu, 27 Oct 2022 19:46:30 +0200 (CEST) Received: by ktus.lan (Postfix, from userid 209) id 9B0B630DF8F; Thu, 27 Oct 2022 19:46:30 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-Spam-Level: X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,KAM_DMARC_STATUS,NICE_REPLY_A,SPF_FAIL,SPF_HELO_NONE,TXREP 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 AD81A30DF8B; Thu, 27 Oct 2022 19:46:23 +0200 (CEST) Message-ID: Date: Thu, 27 Oct 2022 19:46:15 +0200 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 To: Luis Machado , gdb-patches@sourceware.org References: <1666353538-15846-1-git-send-email-vanekt@fbl.cz> <8c72008e-6151-846d-77dd-7da642b1489f@arm.com> From: Tomas Vanek In-Reply-To: <8c72008e-6151-846d-77dd-7da642b1489f@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit List-Id: 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