reproduced using git/master/HEAD

GNU gdb (GDB) 12.0.50.20211102-git
...
This GDB was configured as "--host=x86_64-pc-linux-gnu --target=arm-none-eabi"

according to ARM: ref https://developer.arm.com/documentation/ka001181/latest

quote:
 > During LOCKUP, the PC will be forced to 0xFFFFFFFx and will keep fetching from that address

applying this minor change to test the theory seem to work perfectly:

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 5b60831bbe9..930eba7155e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -543,6 +543,7 @@ arm_m_addr_is_magic (CORE_ADDR addr)
 {
   switch (addr)
     {
+      case 0xfffffffe:
       /* Values from ARMv8-M Architecture Technical Reference.  */
       case 0xffffffb0:
       case 0xffffffb8:



now gdb reports:

(gdb) tar ext :3333
Remote debugging using :3333
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
<signal handler called>
(gdb) c
Continuing.
stm32h7x.cpu0 -- clearing lockup after double fault

Program received signal SIGINT, Interrupt.
<signal handler called>
(gdb)

It seems fairly obvious this magic PC needs to be handled, but is it right to display something like <signal handler called>?

I agree it might be some mixups between LR and PC, though I think in practice it might not cause any problems in these cases.

To cover also Cortex-M33 ARMv8M maybe also check for LR=0xFFFFFFFE and PC=0xEFFFFFFE. The 'easy' fix is to add these values I guess:

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 857a52a9a51..33b03ead5a4 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -556,6 +556,13 @@ arm_m_addr_is_magic (CORE_ADDR addr)
       case 0xfffffff1:
       case 0xfffffff9:
       case 0xfffffffd:
+      /* 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:
+      case 0xfffffffe:
+      case 0xffffffff:
        /* Address is magic.  */
        return 1;

Though maybe we would like to add some more info that actually detects more explicit the LOCKUP state and make some printouts about that?

0xfffffffe is just one missing magic address.
according to arm: 
> During LOCKUP, the PC will be forced to 0xFFFFFFFx

so all 0xFFFFFFFx should be considered magic as well, and this hides several EXC_RETURN

I'm wondering why we are linking this to EXC_RETURN, since this function is used to check PC values and EXC_RETURN values are loaded into LR.

more literature:
according to ARM v6M Arch ref man (ARM DDI 0419E page B1-198),
section B1.5.8: Exception return behavior:
> An exception return occurs when the processor is in Handler mode
> and one of the following instructions loads a value of 0xFXXXXXXX into the PC

according to this all 0xfxxxxxxx values are magic
the same text is present in ARM v7M arch ref man

in ARM v8M ref man, section B3.22:
> ... loads an EXC_RETURN value, 0xFFXXXXXX, into the PC


============================================

regarding: 
is it right to display something like <signal handler called>?
it's fine for me, unless there is a way to display something similar to "exception" 

but I agree, if we want to have a minor fix/change:
we can limit to adding the 3 magic addresses:
+      case 0xeffffffe:
+      case 0xfffffffe:
+      case 0xffffffff:

Created attachment 13760 [details]
Proposed patch

Proposed patch

This might started with patch from 2016-09-27:

@@ -472,7 +528,7 @@ arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
   /* On M-profile devices, do not strip the low bit from EXC_RETURN
      (the magic exception return address).  */
   if (gdbarch_tdep (gdbarch)->is_m
-      && (val & 0xfffffff0) == 0xfffffff0)
+      && arm_m_addr_is_magic (val))
     return val;
 
   if (arm_apcs_32)

Though still PC=0xEFFFFFFE is a Lockup state for ARMv8M to my understanding, so just revert that wont do the full trick. Also I think it is good to have the explicit EXC_RETURN values defined to possibly make way to future special handling of the actual meaning of these values, like using different stack pointers etc.

Created attachment 13761 [details]
Proposed alternative patch

Proposed alternative more aggressive patch, catches all EXC_RETURN 0xFFxxxxxx.

Hi Fredrik,

I have checked both patches they seems to work fine !

I don't have a strong opinion on which one is more suitable, since IDK what arm_addr_bits_remove is used for.

I let you decide which one is better.

Thanks again for your reactivity !

Kind Regards,
Tarek

I think we should keep the magic check function, but update it to do checks for more cases of special PC's.

Do any of you have a copyright assignment in place? Or should I pursue this upstream based on the manual entries?

IMO please proceed with manual entries. unless Fredrik has a different thought.

Either is fine by me, I have copyright assignment in place, actually it was me who originally added this function 'arm_m_addr_is_magic()' in the first place, so I guess you can blame me for the bug ;-)
BR Fredrik

Indeed. I see it now! :-)

In my opinion, the change to arm_addr_bits_remove should really go into arm_m_addr_is_magic. And it would be nice to update the documentation (already quite detailed) to include these previously unhandled magic PC's.

Otherwise, it looks OK to me.

Fredrik,

I'm afraid that simply adding lockup addresses to the list of magic address is not correct. The Cortex-M lockup differs from an exception! The lockup does not stack anything, it just sets PC (and other special regs). So there is no point in trying to unwind lockup frame. It does not seem possible, the previous PC is lost.

Unwinding lockup state as it was an exception may result again in peeking not existing memory (arm_m_exception_cache() reads word @ unwound_sp + 28)

So I would recommend splitting arm_m_addr_is_magic() to arm_m_addr_is_exc_return()
and arm_m_addr_is_lockup().
Then introduce another Cortex-M specific unwinder "arm m lockup" with a trivial implementation:
arm_m_lockup_this_id() will always return outer_frame_id
and
arm_m_lockup_unwind_sniffer() will check PC for arm_m_addr_is_lockup()

Tom

The custom unwinder that just stops seems to be a more elegant solution, indeed.

Luis,
TBH I didn't care about elegance ;)
The prevention of bogus unwinding is much more important.
Look at test results.
Just before real lockup:

Breakpoint 1, DoubleFaultGenerator () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:
72
72            double_fault_dest= *double_fault_unreadable_src;
(gdb) backtrace
#0  DoubleFaultGenerator () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:72
#1  0x0c000a50 in HardFault_Handler ()
    at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:99
#2  <signal handler called>
#3  0x0c000876 in HardFaultGenerator () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:406
#4  0x0c0008c0 in HAL_SYSTICK_Callback () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:427
#5  0x0c000b1e in SysTick_Handler ()
    at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:234
#6  <signal handler called>
#7  HAL_GPIO_ReadPin (GPIOx=0x0, GPIO_Pin=0)
    at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Drivers/STM32L5xx_HAL_Driver/Src/stm32l5xx_hal_gpio.c:371
#8  0x0c0004ec in main () at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/main.c:165

Locked-up, gdb with Fredrik's alternative patch:

(gdb) c
Continuing.
stm32l5x.cpu -- clearing lockup after double fault
 
Program received signal SIGINT, Interrupt.
<signal handler called>
(gdb) backtrace
#0  <signal handler called>
#1  0x4fff0000 in ?? ()
#2  0x30000048 in pSecureErrorCallback ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

As expected, backtrace got completely lost.

I am looking at PR26613, which is probably partly related to this bug report.

What I meant by elegance is making GDB stop when it should and display the right information (that we're locked up, instead of appearing like we stopped in the middle of a signal handler).

If GDB attempts to backtrace further (as you showed in your example), that sounds like a problem with the patch. It should've acted like the following...

--

(gdb) c
Continuing.
stm32h7x.cpu0 -- clearing lockup after double fault

Program received signal SIGINT, Interrupt.
<signal handler called>
(gdb)

--

... which, in my opinion, is not as informative as displaying a custom message saying we're locked up.

To complement, the backtrace produced should not go further than <signal handler called>. This might be a limitation of how we currently handle magic PC's/LR's.

Luis, Fredrik,

I wrote a draft of the proposed "lockup" custom unwinder. I'm not familiar with gdb source so I'm asking for a little help.

I'm very unsure what arm_m_lockup_prev_register() should return.
I think it's not important as the lockup frame declare itself as outer.
Is it correct?

Is there an easy way to return something valid without generating a frame cache?
Perhaps
 return frame_unwind_got_register (this_frame, prev_regnum, prev_regnum);
would be ok?

Tom

Hi all,
would it be possible to handle Lockup inside function
  <arm_m_exception_cache (struct frame_info *this_frame)>

PSEUDO:

     if (pc == 0xFFFFFFFE)  <<== Lockup criterion checked in exception cache
       {
	  warning (_("no thread stack unwinding supported in Lockup."));

	  /* Terminate any further stack unwinding by refer to self.  */
	  cache->prev_sp = sp;
	  return cache;
       }

Something like this (sketch not compiled):

@@ -2982,6 +2989,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   struct arm_prologue_cache *cache;
+  CORE_ADDR pc;
   CORE_ADDR lr;
   CORE_ADDR sp;
   CORE_ADDR unwound_sp;
@@ -2998,12 +3006,26 @@ arm_m_exception_cache (struct frame_info *this_frame)
      describes which bits in LR that define which stack was used prior
      to the exception and if FPU is used (causing extended stack frame).  */
 
+  pc = get_frame_register_unsigned (this_frame, ARM_PC_REGNUM);
   lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
   sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
 
   /* Check EXC_RETURN indicator bits.  */
   exc_return = (((lr >> 28) & 0xf) == 0xf);
 
+  /* Check if the core is in Lockup state.  */
+  if (exc_return && (pc == 0xFFFFFFFE))
+    {
+      /* Thread (process) stack could not be fetched in Lockup,
+        give warning and exit.  */
+
+      warning (_("no thread stack unwinding supported in Lockup."));
+
+      /* Terminate any further stack unwinding by refer to self.  */
+      cache->prev_sp = sp;
+      return cache;
+    }
+
   /* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used.  */
   process_stack_used = ((lr & (1 << 2)) != 0);
   if (exc_return && process_stack_used)

Fredrik,

it does not stop unwinding:

(gdb) bt
#0  <signal arm m exception>
#1  0x4fff0000 in ?? ()
#2  0x30000050 in pSecureErrorCallback ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

IMO returning outer_frame_id is crucial and yes, arm_m_exception_this_id()
can be modified to return it if lockup is detected. In comparison with this a new custom unwinder sniffing for lockup seems me cleaner.

And with a small modification gdb 'backtrace' can print fancy unwinder name instead of just <signal handler called>: 

(gdb) bt
#0  <signal arm m lockup>
(gdb)

Fredrik,

finally I found that exc_return is false in case of ARMv8m lockup (PC 0xeffffffe),
so your code also can stop unwinding (with twice printed warning):

(gdb) bt
warning: no thread stack unwinding supported in Lockup.
warning: no thread stack unwinding supported in Lockup.
#0  <signal handler called>
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)

Hmmm, not neat...

One more test, this time Cortex-M4 just after reset (issued OpenOCD reset halt).
Lockup didn't happen, just the CPU prepared LR=0xffffffff to catch eventual erroneous return from the app:

(gdb) bt
warning: no thread stack unwinding supported in Lockup.
#0  0x080006b8 in Reset_Handler ()
(gdb) adv main
warning: no thread stack unwinding supported in Lockup.
Note: automatically using hardware breakpoints for read-only addresses.
main () at blink.c:164
164     {
(gdb)

Seems me kind of unnecessary noise although nothing wrong happened. Besides that it works correctly, no nonsense unwinds observed.

Created attachment 13852 [details]
Fixes for ARM M exception frame unwinder

Reworked Proposed alternative patch (515 bytes, patch)
2021-11-06 00:20 UTC, Fredrik Hederstierna

- rename detection function to arm_m_addr_is_exc_return
- exclude lockups
- add all possible EXC_RETURN values in ARMv8m including Security Extension
- debug prints
- interstate exceptions (Secure->NonSecure and NonSecure->Secure)
  are not handled, unwinding is terminated

Created attachment 13853 [details]
ARM M lockup frame unwinder

Created attachment 13854 [details]
Show frame unwinder name in signal name

Shows more informative <signal arm m exception>
instead of <signal handler called>

I think this is looking pretty good. Should we take it to the mailing list for some feedback from others?

I also think it looks good! Great job, smart idea with separate unwinder for lockup. /Fredrik

(In reply to Luis Machado from comment #29)
> Should we take it to the mailing list
> for some feedback from others?

Yes please, any feedback welcomed.

Dear All,

FYI,

I have been informed that a colleague (christophe.lyon@foss.st.com) have posted a PATCH series to address a similar issue.

I'm just pointing to the patch (since I haven't tested it yet):

[PATCH 4/5] gdb/arm: Add support for multiple stack pointers on Cortex-M
https://sourceware.org/pipermail/gdb-patches/2022-January/185130.html

[PATCH 5/5] gdb/arm: Extend arm_m_addr_is_magic to support FNC_RETURN, add unwind-ns-to-s command
https://sourceware.org/pipermail/gdb-patches/2022-January/185131.html

Yeah. He has posted a comment previously. I'm going through his series at the moment.

Please let us know how it works on your end. I don't have a proper setup to test this.

Hi, I wanted to update this bug report but you have been faster than me :-)

Note that you'll need patches 1-3 too (well, maybe the prologue analysis is not needed in your case, but skipping it may cause incorrect backtrace)

Created attachment 13916 [details]
attachment-2704047-0.html

Hi Christophe,

Sure thing, previous changes are needed ;)
I have just pointed out the patches with subjects explicitly related to the
issue.

I will give them a try on real crash scenarios and let you know with my
findings.

Thanks !
Tarek


Le mer. 19 janv. 2022 à 12:40, clyon at gcc dot gnu.org <
sourceware-bugzilla@sourceware.org> a écrit :

> https://sourceware.org/bugzilla/show_bug.cgi?id=28549
>
> --- Comment #35 from Christophe Lyon <clyon at gcc dot gnu.org> ---
> Hi, I wanted to update this bug report but you have been faster than me :-)
>
> Note that you'll need patches 1-3 too (well, maybe the prologue analysis
> is not
> needed in your case, but skipping it may cause incorrect backtrace)
>
> --
> You are receiving this mail because:
> You reported the bug.

Was this ever addressed upstream? Is there an updated patch?

I don't think Tomas have rebased these three great patches attached 2021-12-14, and unfortunately I haven't had time to look further into it myself either.
Don't know if any of the developers working with similar functionalities at ST have done any similar patch, or could help or support in trying to improve the lockup state handling using any ideas from these attachments?
Thanks! Kindly, Fredrik

There has been a recent round of contributions by ST around trustzone. Commit ef273377587d touches arm_m_addr_is_magic, but it may not account for all possible return magic codes.

Fredrik is right, I was too busy and completely forgot about this.

Luis, what am I expected to do to get the patch upstreamed?
And could you point to the conflicting changes in frame unwinder code if there is more recent work than that one pointed by Tarek?

Hi Tomas,

You can refer to https://sourceware.org/gdb/wiki/ContributionChecklist for some information on how to contribute. You'll need a FSF copyright assignment in place to be able to contribute non-trivial changes. Do you have one?

As for the change, here's what git shows for that commit:

@@ -695,28 +730,43 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
    0xFFFFFFBC    Return to Thread mode using the process stack.  */
 
 static int
-arm_m_addr_is_magic (CORE_ADDR addr)
-{
-  switch (addr)
-    {
-      /* Values from ARMv8-M Architecture Technical Reference.  */
-      case 0xffffffb0:
-      case 0xffffffb8:
-      case 0xffffffbc:
-      /* Values from Tables in B1.5.8 the EXC_RETURN definitions of
-        the exception return behavior.  */
-      case 0xffffffe1:
-      case 0xffffffe9:
-      case 0xffffffed:
-      case 0xfffffff1:
-      case 0xfffffff9:
-      case 0xfffffffd:
-       /* Address is magic.  */
-       return 1;
+arm_m_addr_is_magic (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+  if (tdep->have_sec_ext)
+    {
+      switch ((addr & 0xff000000))
+       {
+       case 0xff000000: /* EXC_RETURN pattern.  */
+       case 0xfe000000: /* FNC_RETURN pattern.  */
+         return 1;
+       default:
+         return 0;
+       }
+    }
+  else
+    {
+      switch (addr)
+       {
+         /* Values from ARMv8-M Architecture Technical Reference.  */
+       case 0xffffffb0:
+       case 0xffffffb8:
+       case 0xffffffbc:
+         /* Values from Tables in B1.5.8 the EXC_RETURN definitions of
+            the exception return behavior.  */
+       case 0xffffffe1:
+       case 0xffffffe9:
+       case 0xffffffed:
+       case 0xfffffff1:
+       case 0xfffffff9:
+       case 0xfffffffd:
+         /* Address is magic.  */
+         return 1;
 
-      default:
-       /* Address is not magic.  */
-       return 0;
+       default:
+         /* Address is not magic.  */
+         return 0;
+       }
     }
 }

(In reply to Luis Machado from comment #41)

> You'll need a FSF copyright
> assignment in place to be able to contribute non-trivial changes. Do you
> have one?

No, I do not.

> As for the change, here's what git shows for that commit:

Thanks, I've already seen the code in the current git master.
I asked to find if there is some new pending work not yet merged to master branch.

(In reply to tomas.vanek from comment #42)
> (In reply to Luis Machado from comment #41)
> 
> > You'll need a FSF copyright
> > assignment in place to be able to contribute non-trivial changes. Do you
> > have one?
> 
> No, I do not.
> 

You will need to get one (or be covered by one by your employer, if there is one).

More information here: https://sourceware.org/gdb/wiki/ContributionChecklist, item 6.

If you don't want to do it, someone else with copyright assignment in place will need to write their own code to implement this change.

> > As for the change, here's what git shows for that commit:
> 
> Thanks, I've already seen the code in the current git master.
> I asked to find if there is some new pending work not yet merged to master
> branch.

Ah, sorry. I misunderstood. No, I don't think there are further changes planned for this particular piece of code.

(In reply to tomOas.vanek from comment #42)
> (In reply to Luis Machado from comment #41)
> 
> > You'll need a FSF copyright
> > assignment in place to be able to contribute non-trivial changes. Do you
> > have one?
> 
> No, I do not.

Any progress on the FSF copyright?

> > As for the change, here's what git shows for that commit:
> 
> Thanks, I've already seen the code in the current git master.
> I asked to find if there is some new pending work not yet merged to master
> branch.

I have some more pending patches for Cortex-M unwinding, but nothing that touches the arm_m_addr_is_magic. However, I don't think you need to modify this function.
I did try attachment 13853 [details] a few weeks ago on top of the other changes that I've submitted so far and it worked. I have some minor comments on it, that I shared in the private mail-thread with subject "Cortex-M lockup patch question" that Luis CCed you on.
If you send attachment 13853 [details] to gdb-patches@soureware.org, I can reply with my comments inline in you patch and we can take it from there?

(In reply to Torbjörn SVENSSON from comment #44)
> 
> Any progress on the FSF copyright?

I sent the signed form 2 weeks ago.
I'm still waiting for a reply with approval indication.

Hi Tomas,

Could you please ping copyright-clerk@fsf.org about the status of your assignment?

I checked this morning and it seems your copyright isn't in place yet.

(In reply to tomas.vanek from comment #45)
> (In reply to Torbjörn SVENSSON from comment #44)
> > 
> > Any progress on the FSF copyright?
> 
> I sent the signed form 2 weeks ago.
> I'm still waiting for a reply with approval indication.

From what I heard, the FSF copyright process has now completed.

Tomas, can you please send your patch to the gdb-patches ML in order to get it merged after a review process? If it helps, I can help with adapting the patch for the review comments, but you need to send the initial version.

(In reply to Torbjörn SVENSSON from comment #47)
> 
> From what I heard, the FSF copyright process has now completed.

Yes, it's finally done. Sorry, it was my fault: I replied to copyright-clerk@fsf.org first instead of sending the signed form to assign@gnu.org

> Tomas, can you please send your patch to the gdb-patches ML in order to get
> it merged after a review process? If it helps, I can help with adapting the
> patch for the review comments, but you need to send the initial version.

Will do.

The discussion here narrowed to 'ARM M lockup frame unwinder' patch. I originally attached 3 patches, see above. TBH I never intended to use just
the unwinder without accompanying changes. 
'Show frame unwinder name in signal name' gives user little bit better info than <signal handler called> - it might be mergeable (I didn't check, will do).
The rest of work in 'Fixes for ARM M exception frame unwinder' is probably definitely unusable because of merging ARMv8 profile M security extension support.
I need some time to check the new code and see if some parts of my old changes are applicable.

Back to 'ARM M lockup frame unwinder' and Torbjörn's comments Fredrik forwarded me by mail:

> 1. arm_m_lockup_prev_register
> I think arm_m_lockup_prev_register should have an assert statement as it
> should never be reached unless the sniffer is incorrectly implemented.

I think the same but I was not able to find the part of the general frame code (frame-unwind.c, frame.c ...) which ensures it. That's why I wanted to avoid gdb fail or assert because post-mortem debugging of frame related structures seemed me almost impossible.
So if you confirm that prev_register method cannot be called when this_id method returned outer_frame in any corner case like sniffing of corrupted data, then ok, let's just assert(0) in prev_register

> 2. arm_m_lockup_unwind_sniffer
> I would have inlined the arm_m_addr_is_lockup function here as there is
> only one call to it and it would make it easier to see what is happening.

I just kept the format of original arm_m_addr_is_magic().
Maybe keeping all tests for 'special' addresses together has some advantage in code readability.

> 3. registering the lockup unwinder
> I think it makes sense to have the lockup unwinder as early in the list
> as possible to avoid any of the other unwinders to preserve the state of
> the target as much as possible

Well the lockup unwinder gets registered with the exception unwinder as the first two unwinders. You might miss the code from 'Fixes for ARM M exception frame unwinder' - with this patch, lockup and exception are sniffed as mutually exclusive, no matter which one is sniffed first. I agree, without the second patch the lockup unwinder registration should precede the exception unwinder,
especially with ARMv8 profile M security extension with arm_m_addr_is_magic() catching lockup magic codes.

I looked to the new code to support ARMv8 profile M security extension just very shortly.
I wonder why arm_m_addr_is_magic() decoding differs so much in branches without and with the security extension. The former decodes just the used special addresses, the latter whole pages with lot of 'undefined behaviour' codes.
IMO the frame unwinding should proceed as long as sniffers encounter frames possibly saved by the processor and stop as soon as a sniffer hits something suspicious like a special address described as 'undefined behaviour' in architecture manual. Stopping frame unwinding seems me much better than generating nonsense frames from corrupted data.
Considering this I think we need another meta unwinder - or better said unwind stopper for 'undefined behaviour'. Registered as the last one and catching everything what all other sniffers refuse.
What do you think?

(In reply to tomas.vanek from comment #48)
> The discussion here narrowed to 'ARM M lockup frame unwinder' patch. I
> originally attached 3 patches, see above. TBH I never intended to use just
> the unwinder without accompanying changes. 

Right now, I'm more concerned about the endless(?) loop when the target is
in lockup state. I think we should first solve that problem and then look
into if there is improvements to do to make it more obvious for the user
what's going on.

> 'Show frame unwinder name in signal name' gives user little bit better info
> than <signal handler called> - it might be mergeable (I didn't check, will
> do).

I think this patch could be an improvement, but not as important as the
previous one. Lets focus on getting the other one in first, okay?

> The rest of work in 'Fixes for ARM M exception frame unwinder' is probably
> definitely unusable because of merging ARMv8 profile M security extension
> support.

I don't think any of the other changes are needed anymore.

> I need some time to check the new code and see if some parts of my old
> changes are applicable.
> 
> Back to 'ARM M lockup frame unwinder' and Torbjörn's comments Fredrik
> forwarded me by mail:
> 
> > 1. arm_m_lockup_prev_register
> > I think arm_m_lockup_prev_register should have an assert statement as it
> > should never be reached unless the sniffer is incorrectly implemented.
> 
> I think the same but I was not able to find the part of the general frame
> code (frame-unwind.c, frame.c ...) which ensures it. That's why I wanted to
> avoid gdb fail or assert because post-mortem debugging of frame related
> structures seemed me almost impossible.
> So if you confirm that prev_register method cannot be called when this_id
> method returned outer_frame in any corner case like sniffing of corrupted
> data, then ok, let's just assert(0) in prev_register

I just landed a patch that shows how you can use the 
frame_unwind_stop_reason hook to stop the unwinding. I think the same logic
could be used in your patch too.
Regarding the assurance, please see Pedro's reply to the very same question
here: https://sourceware.org/pipermail/gdb-patches/2022-October/192655.html

> > 2. arm_m_lockup_unwind_sniffer
> > I would have inlined the arm_m_addr_is_lockup function here as there is
> > only one call to it and it would make it easier to see what is happening.
> 
> I just kept the format of original arm_m_addr_is_magic().
> Maybe keeping all tests for 'special' addresses together has some advantage
> in code readability.
> 
> > 3. registering the lockup unwinder
> > I think it makes sense to have the lockup unwinder as early in the list
> > as possible to avoid any of the other unwinders to preserve the state of
> > the target as much as possible
> 
> Well the lockup unwinder gets registered with the exception unwinder as the
> first two unwinders. You might miss the code from 'Fixes for ARM M exception
> frame unwinder' - with this patch, lockup and exception are sniffed as
> mutually exclusive, no matter which one is sniffed first. I agree, without
> the second patch the lockup unwinder registration should precede the
> exception unwinder,
> especially with ARMv8 profile M security extension with
> arm_m_addr_is_magic() catching lockup magic codes.
> 
> I looked to the new code to support ARMv8 profile M security extension just
> very shortly.
> I wonder why arm_m_addr_is_magic() decoding differs so much in branches
> without and with the security extension. The former decodes just the used
> special addresses, the latter whole pages with lot of 'undefined behaviour'
> codes.

On https://developer.arm.com/documentation/100701/0200/The-EXC-RETURN-register,
it's written as that 0xffxxxxxx is an EXC_RETURN pattern, even if there are
combinations within that that is illegal.

> IMO the frame unwinding should proceed as long as sniffers encounter frames
> possibly saved by the processor and stop as soon as a sniffer hits something
> suspicious like a special address described as 'undefined behaviour' in
> architecture manual. Stopping frame unwinding seems me much better than
> generating nonsense frames from corrupted data.

I agree completely with this statement.

> Considering this I think we need another meta unwinder - or better said
> unwind stopper for 'undefined behaviour'. Registered as the last one and
> catching everything what all other sniffers refuse.
> What do you think?

I don't think we need another unwinder, but we might need to have some more
checks in the arm_m_exception_cache function to stop the unwind in case an
unsupported state is detected.

I finally submitted
https://sourceware.org/pipermail/gdb-patches/2022-October/192716.html

(In reply to Torbjörn SVENSSON from comment #49)
> 
> Right now, I'm more concerned about the endless(?) loop when the target is
> in lockup state.

Could it be explained by unwinding with wrong SP value because the size
of the next outer frame cannot be determined because of unknown pre-lockup PC?
 
> > 'Show frame unwinder name in signal name' gives user little bit better info
> > than <signal handler called> - it might be mergeable (I didn't check, will
> > do).
> 
> I think this patch could be an improvement, but not as important as the
> previous one. Lets focus on getting the other one in first, okay?

Because you doesn't seem to be happy with
'Show frame unwinder name in signal name'...

> I just landed a patch that shows how you can use the 
> frame_unwind_stop_reason hook to stop the unwinding. I think the same logic
> could be used in your patch too.

... and your patch made much easier to stop unwinding without misleading
messages, I simplified the change. Dedicated lockup unwinder is dropped,
I added the equal functionality to the exception unwinder.

TBH Personally I would prefer 3 dedicated unwinders: exception (EXC_RETURN), FNC_RETURN and lockup. I think that performance penalty would be negligible
as the sniffers are simple and fast and the code would be cleaner. However I'm not so familiar with gdb code so I respect your preference.

BTW Thanks for the pointer to
> Regarding the assurance, please see Pedro's reply to the very same question
> here: https://sourceware.org/pipermail/gdb-patches/2022-October/192655.html

and submitting your
'[PATCH v4] gdb/arm: Stop unwinding on error, but do not assert'
and
'[PATCH] gdb/arm: Don't rely on loop detection to stop unwind'

They really make a difference.
But there is a small drawback: as the warning messages are printed out in time
of building cache, they appear just once and if user e.g. repeats 'backtrace'
command he gets no more messages.


> > I wonder why arm_m_addr_is_magic() decoding differs so much in branches
> > without and with the security extension. The former decodes just the used
> > special addresses, the latter whole pages with lot of 'undefined behaviour'
> > codes.
> 
> On
> https://developer.arm.com/documentation/100701/0200/The-EXC-RETURN-register,
> it's written as that 0xffxxxxxx is an EXC_RETURN pattern, even if there are
> combinations within that that is illegal.

Hmm, this document really looks like the devices without sec ext have just
3 listed EXC_RETURNs. 'Arm®v8-M Architecture Reference Manual', D1.2.95 'EXC_RETURN, Exception Return Payload' makes clear that the payload format and bits assignment are same for both with and without sec ext, just some bits are reserved in the case 'without sec'.
So IMO detecting EXC_RETURN on a device with security extension should treat
reserved bits same way as doing so on dev w/o sec, not to do the quite opposite.

And one more suspicious point: ARMv8-M RM describes the FType bit 4
as it does not depend on sec ext. Correct me if I'm wrong it would mean
that ARMv8-M device with FP and without security used also 0xFFFFFFA0,
0xFFFFFFA8 and 0xFFFFFFAC EXC_RETURNs. I don't have such device
neither I'm not aware of any, it even does not exist at all.

BTW it's quite easy to trigger the internal error at the end of
arm_m_exception_cache(). Just set PC register to any of magic codes
(I admit that doing so has little sense except showing another reason to check the reserved bits):

On STM32L5 ARMv8-M with sec ext:

(gdb) set $pc=0xff000000
../../gdb/arm-tdep.c:3684: internal-error: While unwinding an exception frame, found unexpected Link Register value 0c0002cb.  This should not happen and maybe caused by corrupt data or a bug in GDB.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
0x8119852 gdb_internal_backtrace_1
        ../../gdb/bt-utils.c:122
0x8119852 _Z22gdb_internal_backtracev
        ../../gdb/bt-utils.c:168
0x843a40c internal_vproblem
        ../../gdb/utils.c:396
0x843a79e _Z15internal_verrorPKciS0_Pc
        ../../gdb/utils.c:476
0x8528326 _Z14internal_errorPKciS0_z
        ../../gdbsupport/errors.cc:58
0x80cf181 arm_m_exception_cache
        ../../gdb/arm-tdep.c:3689
0x80cff77 arm_m_exception_this_id
        ../../gdb/arm-tdep.c:3703
0x8257bbe compute_frame_id
        ../../gdb/frame.c:594
0x8257e49 _Z12get_frame_id14frame_info_ptr
        ../../gdb/frame.c:637
0x825b185 _Z16frame_find_by_id8frame_id
        ../../gdb/frame.c:906
0x8447bbf _Z12value_assignP5valueS0_
        ../../gdb/valops.c:1304
0x8226a46 _ZN10expression8evaluateEP4type6noside
        ../../gdb/eval.c:101
0x8226e8e _Z19evaluate_expressionP10expressionP4type
        ../../gdb/eval.c:115
0x8328a4e set_command
        ../../gdb/printcmd.c:1519
0x814b1aa _Z8cmd_funcP16cmd_list_elementPKci
        ../../gdb/cli/cli-decode.c:2543
0x84001bb _Z15execute_commandPKci
        ../../gdb/top.c:692
0x822dff4 _Z15command_handlerPKc
        ../../gdb/event-top.c:616
0x822e366 _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
        ../../gdb/event-top.c:860

On Kinetis K22, Cortex-M4 FP:

(gdb) set $pc=0xFFFFFFBC
the same error follows...

Patch under review.

IIUC, is the following commit fixing this PR allowing us to close it?

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8b73ee207c9c4b2d692a8
2a29d1cee2dcfa07394

According to Torbjorn's email (https://sourceware.org/pipermail/gdb-patches/2022-October/193182.html), he said all his items for the GDB 13 release have been handled, and this was one of them, so hoping I found the right patch to close this PR...

Thanks!

Joel,

Tomas' patch was pushed (although unfortunately incorrectly authored due to me missing the --author tweak) to fix this. It should be safe to close it.