From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) by sourceware.org (Postfix) with ESMTPS id BC4E83856974 for ; Fri, 21 Oct 2022 18:56:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BC4E83856974 Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 29LCWCFR018413; Fri, 21 Oct 2022 20:56:38 +0200 Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3kbuh2htxj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 21 Oct 2022 20:56:38 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id BB0F4100034; Fri, 21 Oct 2022 20:56:33 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 86FA92C38CA; Fri, 21 Oct 2022 20:56:33 +0200 (CEST) Received: from [10.252.25.190] (10.252.25.190) by SHFDAG1NODE3.st.com (10.75.129.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Fri, 21 Oct 2022 20:56:33 +0200 Message-ID: <33a3ce23-b2db-a1b0-01d3-8a3e9668495e@foss.st.com> Date: Fri, 21 Oct 2022 20:56:32 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Subject: Re: [PATCH v3] gdb/arm: Terminate frame unwinding in M-profile lockup state Content-Language: en-US To: Luis Machado , Tomas Vanek , References: <1666035056-19619-1-git-send-email-vanekt@fbl.cz> From: Torbjorn SVENSSON In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.252.25.190] X-ClientProxiedBy: SHFCAS1NODE2.st.com (10.75.129.73) To SHFDAG1NODE3.st.com (10.75.129.71) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-10-21_04,2022-10-21_01,2022-06-22_01 X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Oct 2022 18:56:45 -0000 Hi, On 2022-10-21 11:20, Luis Machado wrote: > On 10/17/22 20:30, Tomas Vanek wrote: >> In the lockup state the PC value of the the outer frame is irreversibly >> lost. The other registers are intact so LR likely contains >> PC of some frame next to the outer one, but we cannot analyze >> the nearest outer frame without knowing its PC >> therefore we do not know SP fixup for this frame. >> >> The frame unwinder possibly gets mad due to the wrong SP value. >> To prevent problems terminate unwinding if PC contains the magic >> value of the lockup state. >> >> Example session wihtout this change, >> Cortex-M33 CPU in lockup, gdb 13.0.50.20221016-git: >> ---------------- >>    (gdb) c >>    Continuing. >> >>    Program received signal SIGINT, Interrupt. >>    0xeffffffe in ?? () >>    (gdb) bt >>    #0  0xeffffffe in ?? () >>    #1  0x0c000a9c in HardFault_Handler () >>        at >> C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:99 >>    #2  0x2002ffd8 in ?? () >>    Backtrace stopped: previous frame identical to this frame (corrupt >> stack?) >>    (gdb) >> ---------------- >> The frame #1 is at correct PC taken from LR, #2 is a total nonsense. >> >> With the change: >> ---------------- >>    (gdb) c >>    Continuing. >> >>    Program received signal SIGINT, Interrupt. >>    warning: ARM M in lockup state, stack unwinding terminated. >>    >>    (gdb) bt >>    #0  >>    (gdb) >> ---------------- >> >> There is a visible drawback of emitting a warning in a cache buildnig >> routine >> as introduced in Torbjörn SVENSSON's >> [PATCH v4] gdb/arm: Stop unwinding on error, but do not assert >> The warning is printed just once and not repeated on each backtrace >> command. >> >> v2 update: warning supressed for other frames than the innermost one. >> v3 update: boolean values and comment fixes >> >> Signed-off-by: Tomas Vanek >> --- >>   gdb/arm-tdep.c | 55 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++--- >>   1 file changed, 52 insertions(+), 3 deletions(-) >> >> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >> index b5facae..aefd241 100644 >> --- a/gdb/arm-tdep.c >> +++ b/gdb/arm-tdep.c >> @@ -724,9 +724,30 @@ class target_arm_instruction_reader : public >> arm_instruction_reader >>     return 0; >>   } >> +static inline bool >> +arm_m_addr_is_lockup (CORE_ADDR addr) >> +{ >> +  switch (addr) >> +    { >> +      /* Values for lockup state. >> +     For more details see "B1.5.15 Unrecoverable exception cases" in >> +     both ARMv6-M and ARMv7-M Architecture Reference Manuals, or >> +     see "B4.32 Lockup" in ARMv8-M Architecture Reference Manual.  */ >> +      case 0xeffffffe: Just a minor question: I think the documentation mentioned that the last bit can be either 0 or 1 and they should both be considered the same instruction as the bit is only for thumb mode. Maybe we should also list 0xefffffff? >> +      case 0xfffffffe: >> +      case 0xffffffff: >> +    return true; >> + >> +      default: >> +    /* Address is not lockup.  */ >> +    return false; >> +    } >> +} >> + >>   /* Determine if the address specified equals any of these magic return >>      values, called EXC_RETURN, defined by the ARM v6-M, v7-M and v8-M >> -   architectures. >> +   architectures. Also include lockup magic PC value. > > Formatting: Two spaces after '.' > >> +   Check also for FNC_RETURN if we have v8-M security extension. > > we have -> we have the > >>      From ARMv6-M Reference Manual B1.5.8 >>      Table B1-5 Exception return behavior >> @@ -769,6 +790,9 @@ class target_arm_instruction_reader : public >> arm_instruction_reader >>   static int >>   arm_m_addr_is_magic (struct gdbarch *gdbarch, CORE_ADDR addr) >>   { >> +  if (arm_m_addr_is_lockup (addr)) >> +    return 1; >> + >>     arm_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); >>     if (tdep->have_sec_ext) >>       { >> @@ -3355,6 +3379,30 @@ struct frame_unwind arm_stub_unwind = { >>        describes which bits in LR that define which stack was used prior >>        to the exception and if FPU is used (causing extended stack >> frame).  */ >> +  /* In the lockup state PC contains a lockup magic value. >> +     The PC value of the the next outer frame is irreversibly >> +     lost. The other registers are intact so LR likely contains > > Formatting: Two spaces after '.'. > >> +     PC of some frame next to the outer one, but we cannot analyze >> +     the next outer frame without knowing its PC >> +     therefore we do not know SP fixup for this frame. >> +     Some heuristics to resynchronize SP might be possible. >> +     For simplicity just terminate unwinding to prevent the unwinder >> +     going mad.  */ > > How about... > > For simplicity, just terminate the unwinding to prevent it going astray and > attempting to read data/addresses it shouldn't, which may cause further > issues due > to side-effects. > > Does that make sense? > >> +  CORE_ADDR pc = get_frame_pc (this_frame); >> +  if (arm_m_addr_is_lockup (pc)) >> +    { >> +      /* The lockup can be real just in the innermost frame >> +     as the CPU is stopped and cannot create more frames. >> +     If we hit lockup magic PC in the other frame, it is >> +     just a sentinel at the top of stack: do not warn then.  */ >> +      if (frame_relative_level (this_frame) == 0) >> +    warning (_("ARM M in lockup state, stack unwinding terminated.")); >> + >> +      /* Terminate any further stack unwinding.  */ >> +      arm_cache_set_active_sp_value (cache, tdep, 0); >> +      return cache; >> +    } >> + >>     CORE_ADDR lr = get_frame_register_unsigned (this_frame, >> ARM_LR_REGNUM); >>     /* ARMv7-M Architecture Reference "A2.3.1 Arm core registers" >> @@ -3824,11 +3872,12 @@ struct frame_unwind arm_stub_unwind = { >>     return arm_m_addr_is_magic (gdbarch, this_pc); >>   } >> -/* Frame unwinder for M-profile exceptions.  */ >> +/* Frame unwinder for M-profile exceptions (EXC_RETURN on stack), >> +   lockup and secure/nonsecure interstate function calls >> (FNC_RETURN).  */ >>   struct frame_unwind arm_m_exception_unwind = >>   { >> -  "arm m exception", >> +  "arm m exception lockup sec_fnc", >>     SIGTRAMP_FRAME, >>     arm_m_exception_frame_unwind_stop_reason, >>     arm_m_exception_this_id, > > Torbjörn, have you managed to exercise this on your end? Does it behave > as expected (stops unwinding gracefully)? I've tested it a bit on a STM32F4-Discovery board and it works better than current HEAD. Based on this board, I'd say LGTM. > > From looking at the code, this looks OK to me, but I don't have the > proper setup to exercise it.