RE: [PATCH 6/9] gdb: Implement 'bt shadow' to print the shadow stack backtrace.

["Schimpe, Christina" <christina.schimpe@intel.com>]

HI Thiago, 

Please find my comments below.

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Mittwoch, 26. November 2025 05:07
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 6/9] gdb: Implement 'bt shadow' to print the shadow
> stack backtrace.
> 
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> 
> > Hi Thiago,
> >
> > Thanks for taking the time to provide this review—I really appreciate your
> detailed input!
> 
> You're most welcome. Thank you for pushing the shadow stack feature
> forward!
> 
> > I’ve implemented part of your feedback, and there are still a few open
> points I’d like to clarify.
> > I hope I didn’t miss any of your comments, as the review has become quite
> comprehensive.
> 
> Indeed, the review ended up more detailed than I anticipated because I'm
> using the review process to also develop the AArch64 side. This made me dig
> into the code quite a bit.
> 
> > Please find my responses and open questions below. Looking forward to
> your thoughts!
> 
> Thanks!
> 
> >> -----Original Message-----
> >> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> >> Sent: Freitag, 31. Oktober 2025 05:02
> >> To: Schimpe, Christina <christina.schimpe@intel.com>
> >> Cc: gdb-patches@sourceware.org
> >> Subject: Re: [PATCH 6/9] gdb: Implement 'bt shadow' to print the
> >> shadow stack backtrace.
> >>
> >> Christina Schimpe <christina.schimpe@intel.com> writes:
> >>
> >> > Example for the output of 'bt shadow' on amd64 linux:
> >> > ~~
> >> > (gdb) bt shadow
> >> > /#0  0x000000000040111f in call1 at amd64-shadow-stack.c:14
> >> > /#1  0x000000000040112f in main at amd64-shadow-stack.c:21
> >> > /#2  0x00007ffff7c3fe70 in __libc_start_call_main at
> >> > ../sysdeps/nptl/libc_start_call_main.h:58
> >> > /#3  0x00007ffff7c3ff20 in __libc_start_main_impl at
> >> > ../csu/libc-start.c:128
> >> > /#4  0x0000000000401045 in _start
> >> > ~~
> >>
> >> Here's an example output in my aarch64-linux test VM:
> >>
> >> (gdb) bt shadow
> >> #0  0x0000aaaaaaaa08ac in call2 at aarch64-gcs-return.c:65
> >> #1  0x0000aaaaaaaa08c0 in call1 at aarch64-gcs-return.c:71
> >> #2  0x0000aaaaaaaa09d4 in main at aarch64-gcs-return.c:110
> >> #3  0x0000000000000000 in ??
> >>
> >> And here is the regular backtrace at the same point:
> >>
> >> (gdb) bt
> >> #0  call3 () at aarch64-gcs-return.c:58
> >> #1  0x0000aaaaaaaa08ac in call2 () at aarch64-gcs-return.c:64
> >> #2  0x0000aaaaaaaa08c0 in call1 () at aarch64-gcs-return.c:70
> >> #3  0x0000aaaaaaaa09d4 in main () at aarch64-gcs-return.c:107
> >>
> >> I have a few comments on this output, at least as it is appears for
> >> GCS. I'm assuming x86 shadow stack output is similar:
> >>
> >> 1. The first thing that I notice is that the number of the frames
> >> don't match between the regular stack and the shadow stack: frame 0
> >> in the regular stack doesn't exist in the shadow stack, regular frame
> >> 1 is shadow frame 0, and so on.
> >>
> >> I think this is confusing. It makes sense that frame 0 doesn't appear
> >> in the shadow stack backtrace because there really isn't an entry for
> >> it in the shadow stack, but I think the shadow entries should start
> >> with number 1, to be consistent.
> >
> > Mh, it's probably better to align, I agree.
> > I'll go with starting frame #1 in the v2, and add a comment about that
> > in the commit message.

Thinking about this again I wonder if it's a good idea to start the numbering with #1. 
Besides frame #0, there are multiple reasons why the numbering of the normal backtrace
is out-of-sync with the shadow stack backtrace, e.g. inline calls, tail calls or python frame
filters.

Another option is to add the original #0 frame using the current $pc. But what I don’t like
about this, is that the user could be confused and think that $pc is on the shadow stack.

What do you think?

> >> 2. The line numbers corresponding to the return addresses don't match
> >> between the regular backtrace and the shadow backtrace, even though
> >> the return addresses are the same. I would say this is a bug. Perhaps
> >> there can be a test making sure they match, if it's not too complicated?
> >
> > The reason for that is explained in a comment in
> do_print_shadow_stack_frame_info:
> >
> >   /* In contrast to find_frame_sal which is used for the ordinary backtrace
> >      command, we always want to print the line that is actually referred
> >      to by the address in the linetable.  That's why we always pass 0 here
> >      as second argument.  */
>
> I don't understand why we want to always print the line that is actually
> referred to by the address in the linetable. find_frame_sal has this
> comment:
> 
>   /* If FRAME is not the innermost frame, that normally means that
>      FRAME->pc points at the return instruction (which is *after* the
>      call instruction), and we want to get the line containing the
>      call (because the call is where the user thinks the program is).
> 
> Doesn't that also apply to the addresses in the shadow stack?

Currently in v1 of this series, I print the line containing the return address
which results in the behaviour you describe in point 2 above: "The line
numbers corresponding to the return addresses don't match between the
regular backtrace and the shadow backtrace, ...".  If the user runs
"bt -shadow", I'd expect that he is familiar with the concept of shadow stacks
and I thought it would confuse him more if we show a line which is not a
return address.  But on the other hand, we want to align with the normal
backtrace and print the line containing the call...

> > Do you think we should align this with the behavior of the ordinary
> > backtrace command here as well?
> 
> I think so, but maybe there's something I'm not considering.

...but since the overall direction is to align more with the normal backtrace,
I agree with you here. 😊
In case the normal backtrace is broken due for instance a programming error
such as a buffer overflow the user can use "bt -shadow" as alternative.  In that case, 
implementing frame arguments would probably be helpful, too. But if we do this
afterwards, we'll change the default behavior of "bt -shadow". On the other hand,
this series is already big enough...
And, if we go that direction, I wonder if we shouldn't somehow construct frame #0
for the shadow stack. But as described above, I've also some concerns for that.

What do you think?
 
> > I’m not entirely sure if that approach might be too straightforward,
> > since we can’t simply call the get_frame_address_in_block function as
> > the ordinary bt command does when invoking it from find_frame_sal.
> 
> From what I understand of the big comment in get_frame_address_in_block,
> we can create an equivalent function for shadow stack frames: it should
> return an adjusted PC for frames corresponding to normal function calls,
> which are all except:
> 
> 1. sentinel frame
> 2. signal frame
> 3. dummy frame
> 
> These are easy to recognise (or in the case of 3, can be made easy to
> recognise) in the shadow stack:
> 
> 1. Since the shadow stack doesn't have frame #0, this_frame is never the
> sentinel one. And next_frame will be the "sentinel" if this_frame is the first
> one in the shadow stack.
> 2. The Linux kernel on both AArch64 and Intel puts a marker in the shadow
> stack when a signal is handled, so we can look for it in the shadow stack to
> recognize the signal frame.

Yes, this is what we already discussed here IIRC:
https://sourceware.org/pipermail/gdb-patches/2025-November/222588.html

I changed the gdbarch hook is_no_return_address to additionally take a string, 
which is configured to  <sigframe token> in case of signals for the amd64
implementation.

In the other thread you stated also: 
"In this example "<signal handler called>" message would replace the VDSO
function name in entry #0. Also note that it's an actual return address
but we still want to show a custom message for it."

I am not sure yet for x86 how to detect the shadow stack element for which you
want to display <signal handler called>, since this is a normal return address.
How do you plan to detect this for GCS ?

> 3. We can change GDB to also put a marker in the shadow stack when it does
> an inferior function call, and look for it in the shadow stack.

We call shadow_stack_push in generic GDB code. Do you think we could find an
architecture independent marker ?
Then we can also return true with the gdbarch hook is_no_return_address and
set the  string to <function called from gdb>.

I am just not sure if the linux kernel ever decides to extend its functionality for
CET shadow stack and uses the bits that we use for our marker.
In theory the linux kernel could decide to support 32bit shadow stack  for x86
one day, or supervisor shadow stacks – and I am not sure what bits the kernel
might set in the shadow stack elements to support this.
I wonder if we should discuss this in the linux kernel mailing lists.

But if we detect dummy and signal frames with the gdbarch hook is_no_return_address,
we should only call find_sal_for_pc for normal return addresses on the shadow stack. 

So I think it should be safe to simply call find_sal_for_pc as follows:
  symtab_and_line sal = find_sal_for_pc (frame.value, 1);

Or am I missing something ?

> >> > +/* Return true, if PC is in the middle of a statement.  Note that in the
> >> > +   middle of a statement PC range includes sal.end (SAL.PC, SAL.END].
> >> > +   Return false, if
> >> > +   - SAL.IS_STMT is false
> >> > +   - there is no location information associated with this SAL, which
> >> > +   could happen in case of inlined functions
> >> > +   - PC is not in the range (SAL.PC, SAL.END].
> >> > +   This function is similar to stack.c:frame_show_address but is used
> >> > +   to determine if we are in the middle of a statement only, not to
> decide
> >> > +   if we should print a frame's address.  */
> >> > +
> >> > +static bool
> >> > +pc_in_middle_of_statement (CORE_ADDR pc, symtab_and_line sal) {
> >> > +  if (sal.is_stmt == false)
> >> > +    return false;
> >> > +
> >> > +  /* If there is a line number, but no PC, then there is no location
> >> > +     information associated with this sal.  The only way that should
> >> > +     happen is for the call sites of inlined functions (SAL comes from
> >> > +     find_frame_sal).  Otherwise, we would have some PC range if
> >> > + the
> >>
> >> In the case of stack.c:frame_show_address, SAL comes from
> >> find_frame_sal, but in this case it comes from find_pc_line, right?
> >
> > Yes, it comes from find_sal_for_pc (when rebased on latest master,
> find_pc_line was renamed).
> >
> >> Does the comment still apply in this case?
> >
> > Good question, I remember I stumbled over this a couple of times when
> implementing this.
> > In any case, the comment needs to be corrected to "(SAL comes from
> find_sal_for_pc)."
> > If we need this check, I am not sure. In my experiments with inlined
> > functions I could not reproduce that scenario. But I kept it, to be safe.
> 
> From my reading of find_pc_line / find_sal_for_pc, it doesn't return a sal with
> line number but no PC. But I'm not very familiar with this part of the code.
> But I don't mind keeping this check (with an adjusted comment, as you
> mention).

Ok.

> >> > +     SAL came from a line table.  However, as we don't have a frame for
> >> > +     this function we cannot assert (in contrast to
> >> > +     frame_show_address).  */
> >> > +  if (sal.line != 0 && sal.pc == 0 && sal.end == 0)
> >> > +    return false;
> >> > +
> >> > +  return pc > sal.pc && pc <= sal.end; }
> 
> <snip>
> 
> >> > +/* Extract a char array which can be used for printing a reasonable
> >> > +   error message for REASON.  Note that in case REASON has the value
> >> > +   NO_ERROR the returned array is empty.  */
> >> > +
> >> > +static const char *
> >> > +ssp_unwind_stop_reason_to_err_string (ssp_unwind_stop_reason
> >> > +reason) {
> >> > +  switch (reason)
> >> > +    {
> >> > +    case ssp_unwind_stop_reason::no_error:
> >> > +      return _("");
> >>
> >> The empty string doesn't need to be translated
> >
> > True.
> >
> >> All callers make sure that reason isn't no_error, so perhaps just remove
> this case?
> >
> > I am not sure, if I can follow completely. You mean I should remove the case
> and replace
> >   gdb_assert_not_reached ("invalid reason"); with return "" ?
> 
> I mean just remove this case from the switch. Then this function will assert
> whenever reason != ssp_unwind_stop_reason::memory_read_error.

Ok, now the function looks as follows:

static const char *
ssp_unwind_stop_reason_to_err_string (ssp_unwind_stop_reason reason)
{
  switch (reason)
    {
    case ssp_unwind_stop_reason::memory_read_error:
      return _("shadow stack memory read failure");
    }

  gdb_assert_not_reached ("invalid unwind stop reason.");
}

> >> > +    case ssp_unwind_stop_reason::memory_read_error:
> >> > +      return _("shadow stack memory read failure");
> >> > +    }
> >> > +
> >> > +  gdb_assert_not_reached ("invalid reason"); }
> 
> <snip>
> 
> >> > +/*  If possible, return a shadow stack frame info which is COUNT
> elements
> >> > +    above the bottom of the shadow stack.  FRAME should point to the
> top
> >> > +    of the shadow stack.  RANGE is the shadow stack memory range
> >> > +    [start_address, end_address) corresponding to FRAME's shadow
> stack
> >> > +    pointer.  If COUNT is bigger than the number of elements on the
> shadow
> >> > +    stack, return FRAME.
> >>
> >> Here is another place where I find "above/bottom/top" confusing,
> >> since stacks can grow up or grow down (as pointed out in the next
> >> comment in this function), and these terms mean different things in each
> case.
> >
> > Yes, the comment is now
> > "...above the outermost (oldest) element of the shadow stack."
> >
> > I hope this is more straight-forward.
> 
> There's still "above" and "top". A suggestion:
> 
> "If possible, return a shadow stack frame info which is COUNT elements
> newer than the outermost (oldest) element of the shadow stack.  FRAME
> should point to the newest element of the shadow stack..."

I missed to also update you on the following sentences :/ I'll change this comment
anyways again due to the discussion below. 

> 
> >> > +    In case of failure, assign an appropriate ssp_unwind_stop_reason in
> >> > +    FRAME->UNWIND_STOP_REASON.  */
> >> > +
> >> > +static std::optional<shadow_stack_frame_info>
> >> > +get_trailing_outermost_shadow_stack_frame_info
> >> > +  (gdbarch *gdbarch, const std::pair<CORE_ADDR, CORE_ADDR> range,
> >> > +   const ULONGEST count, shadow_stack_frame_info &frame) {
> >> > +  /* Compute the number of bytes on the shadow stack, starting at
> >> > +     FRAME->SSP, which depends on the direction the shadow stack
> >> > +     grows.  */
> >> > +  const int element_size
> >> > +    = gdbarch_shadow_stack_element_size_aligned (gdbarch);
> >> > +  const unsigned long shadow_stack_bytes
> >> > +    = (gdbarch_stack_grows_down (gdbarch))
> >>
> >> The parentheses around the function call are superfluous.
> >
> > Fixed.
> >
> >> > +       ? range.second - frame.ssp : frame.ssp - range.first +
> >> > + element_size;
> >> > +
> >> > +  gdb_assert ((shadow_stack_bytes % element_size) == 0);  const
> >> > + unsigned long shadow_stack_size
> >> > +    = shadow_stack_bytes / element_size;
> >>
> >> This line fits 80 columns and doesn't need to be broken.
> >>
> >> > +  const long level = shadow_stack_size - count;
> >>
> >> In a comment further below you mention that level is 0-based, but
> >> doesn't this expression mean that it's 1-based? Perhaps it's missing
> >> a
> >> "- 1" term?

A shadow stack with 5 elements (shadow_stack_size = 5) has #0-#4 frames.
We call this function in case count is negative (bt -shadow -2) with its absolute
value so the calculation for level is 5-2 = 3.
This is the right level to start with for a shadow stack including frame #3 and #4.

~~~
(gdb) bt -shadow
#0  0x000055555555514a in call1 at amd64-shadow-stack.c:27
#1  0x000055555555515f in main at amd64-shadow-stack.c:38
#2  0x00007ffff7c2a1ca in __libc_start_call_main at ../sysdeps/nptl/libc_start_call_main.h:58
#3  0x00007ffff7c2a28b in __libc_start_main_impl at ../csu/libc-start.c:360
#4  0x0000555555555065 in _start
(gdb) bt -shadow -2
#3  0x00007ffff7c2a28b in __libc_start_main_impl at ../csu/libc-start.c:360
#4  0x0000555555555065 in _start 	
~~~

> >> Also, this doesn't work for GCS because it assumes all elements in
> >> the stack are return addresses. In GCS, the oldest element is a 0
> >> entry The 0 entry is why this patch requires AArch64 to implement its
> >> own gdbarch top_addr_empty_shadow_stack hook.
> >
> > There is clearly an issue with this function.  But I am not sure if I can follow
> here.
> > Shouldn't the issue be fixed when passing LEVEL instead of COUNT, as
> pointed out by you below?
> 
> Passing level instead of count is necessary but not sufficient.
> 
> The function's documentation comment says that it returns "a shadow stack
> frame info which is COUNT elements newer than the outermost
> (oldest) element of the shadow stack". One example:
> 
> Suppose that the shadow stack has 5 elements and count is 2. Then level as
> calculated by your patch is 3 (5 - 2). So frame.ssp will be adjusted by
> 3 elements by the call to update_shadow_stack_pointer further below. This is
> wrong though, because it makes new_ssp point to 1 element newer than the
> oldest element of the shadow stack.
> If level is calculated instead as level = shadow_stack_size - count - 1 then in
> this example it will be 2 and new_ssp will point to 2 elements newer than the
> oldest element of the shadow stack, which matches the documentation
> comment.

Ah, I think now I get your point.:)

We print the outermost 2 frames, as described in the docs
"With a negative COUNT, print outermost -COUNT frames." for "bt -shadow -2".

But the comment of the function get_trailing_outermost_shadow_stack_frame_info
does not match. 

"...return a shadow stack frame info which is COUNT elements
    above the outermost (oldest) element of the shadow stack."

Based on that for "bt -shadow -2",
~~~
#0  0x000055555555514a in call1 at amd64-shadow-stack.c:27
#1  0x000055555555515f in main at amd64-shadow-stack.c:38
#2  0x00007ffff7c2a1ca in __libc_start_call_main at ../sysdeps/nptl/libc_start_call_main.h:58
#3  0x00007ffff7c2a28b in __libc_start_main_impl at ../csu/libc-start.c:360
#4  0x0000555555555065 in _start
~~~
we want to return the frame belonging to #3, so the frame which is COUNT-1 elements
above the oldest element of the shadow stack.

But still prefer to pass COUNT directly, without any further calculations to this function, 
similar as it's done for the normal backtrace in trailing_outermost_frame.  There the
comment is 

/* Return the starting frame needed to handle COUNT outermost frames.  */.

I found this comment a bit confusing back then when I implemented it, but now it makes
perfect sense to me.  I think I'll change it back to that one, does that sound ok to you ?
Now the full comment is:

/*  If possible, return the starting shadow stack frame info needed to handle
    COUNT outermost frames.  FRAME should point to the innermost (newest)
    element of the shadow stack.  RANGE is the shadow stack memory range
    [start_address, end_address) corresponding to FRAME's shadow stack pointer.
    If COUNT is bigger than the number of elements on the shadow stack, return
    FRAME.  In case of failure, assign an appropriate ssp_unwind_stop_reason in
    FRAME->UNWIND_stop_REASON.  */

> But that still doesn't work for AArch64, because every shadow stack there has
> an extra element at the beginning which also needs to be discounted, so in
> this case it should be:
> 
>   const long level = shadow_stack_size - count - 2;
> 
> But then it's wrong for Intel.
> 
> > And if not, do you have an idea how we can fix this?
> 
> I suggest adding a gdbarch method which given RANGE, returns how many
> entries there are in the shadow stack. And to calculate level as:
> 
>   const long level = shadow_stack_size - count - 1;
> 
> Where count is the result of the gdbarch hook.

I agree, I'll add a gdbarch hook for the shadow stack size.

> >> Finally, not a concern for userspace support (and thus this patch
> >> series) but for OS and hypervisor software there's another kind of
> >> GCS entry which is the exception return record that is put on the
> >> stack when an exception is taken. That entry is 32 bytes in size.  I
> >> mention this just to illustrate that calculating the number of
> >> entries in the stack can be non- trivial.
> >
> > Oh, I see. Isn't that already a problem with the current unwinding of
> > the shadow stack pointer for GCS? How would you fix it there ?
> 
> It's not a problem for now because GCS support is only implemented for
> Linux userspace inferiors. It's a bridge to be crossed in the future. :)
> 
> I just mentioned this case to illustrate that assuming that all entries in the
> shadow stack have the same size is fragile. But for now it's good enough, so
> perhaps I shouldn't have mentioned it.
> 
> >> Not sure how to account for these variations in generic code. Maybe
> >> add a new gdbarch method for returning the number of entries in the
> >> shadow stack?
> >
> > To implement this, I believe we have two possible approaches:
> >
> > (1) Assume we can determine the number of elements without unwinding
> > each frame (as it is currently the case). In this scenario, we could
> > introduce a generic gdbarch hook to retrieve the number of elements for
> architectures that require a different calculation.
> >
> > (2) Unwind each shadow stack frame to obtain the trailing outermost
> > frame, similar to how the normal backtrace works.
> >
> > Do you see any additional options?
> 
> No, I can also only think of these.
> 
> > Alternatively, we could keep the current approach for now (without the
> > gdbarch hook, based on the existing calculation) and only revise the
> > implementation if other OS or hypervisor software require different logic—
> once we have the ability to test those cases properly.
> > What are your thoughts?
> 
> The current approach isn't feasible because level needs to be calculated in
> one way for Intel, and a different way for AArch64. I suggest the gdbarch
> hook option.

Yes, I'll add the hook for the shadow stack size.

> >> > +
> >> > +  /* COUNT exceeds the number of elements on the shadow stack.
> Return the
> >> > +     starting shadow stack frame info FRAME.  */  if (level <= 0)
> >> > +    return std::optional<shadow_stack_frame_info> (frame);
> >> > +
> >> > +  CORE_ADDR new_ssp = update_shadow_stack_pointer
> >> > +    (gdbarch, frame.ssp, count, ssp_update_direction::bottom);
> >>
> >> Shouldn't update_shadow_stack_pointer be called with 'level' rather
> >> than 'count' as argument?
> >
> > Ah, you’re absolutely right. I’m certain this worked at some point before I
> posted it.
> > I’m not sure how this issue slipped in—and even more concerning that my
> tests didn’t catch it.
> >
> >> > +  if (gdbarch_stack_grows_down (gdbarch))
> >> > +    gdb_assert (new_ssp < range.second);  else
> >> > +    gdb_assert (new_ssp >= range.first);
> >> > +
> >> > +  CORE_ADDR new_value;
> >> > +  if (!read_shadow_stack_memory (gdbarch, new_ssp, &new_value,
> >> > +				 &frame.unwind_stop_reason))
> >> > +    return {};
> >> > +
> >> > +  return std::optional<shadow_stack_frame_info>
> >> > +    ({new_ssp, new_value, (ULONGEST) level,
> >> > +      ssp_unwind_stop_reason::no_error});
> >>
> >> This line causes a compilation error in arm-linux-gnueabihf, and I
> >> suppose other 32-bit targets as well:
> >>
> >> /home/bauermann/src/binutils-gdb/gdb/shadow-stack.c:471:27: error:
> >> narrowing conversion of ‘(ULONGEST)((long int)level)’ from ‘ULONGEST’
> >> {aka ‘long long unsigned int’} to ‘long unsigned int’ [-Werror=narrowing]
> >>   471 |     ({new_ssp, new_value, (ULONGEST) level,
> >> ssp_unwind_stop_reason::no_error});
> >>       |                           ^~~~~~~~~~~~~~~~
> >>
> >> Also, it fits 80 columns and doesn't need to be broken.
> >
> > Thanks, casting it as follows:
> > (unsigned long) level
> > should hopefully fix this. And then I will have more than 80 characters.
> 
> Ok.
> 
> >> > +}
> >> > +
> >> > +std::optional<shadow_stack_frame_info>
> >> > +shadow_stack_frame_info::unwind_prev_shadow_stack_frame_info
> >> > +  (gdbarch *gdbarch, std::pair<CORE_ADDR, CORE_ADDR> range) {
> >> > +  /* If the user's backtrace limit has been exceeded, stop.  We must
> >> > +     add two to the current level; one of those accounts for
> >> > +     backtrace_limit being 1-based and the level being 0-based, and the
> >> > +     other accounts for the level of the new frame instead of the level
> >> > +     of the current frame.  */
> >> > +  if (this->level + 2 > user_set_backtrace_options.backtrace_limit)
> >> > +    return {};
> >> > +
> >> > +  CORE_ADDR new_ssp
> >> > +    = update_shadow_stack_pointer (gdbarch, this->ssp, 1,
> >> > +				   ssp_update_direction::bottom);
> >> > +
> >> > +  if (gdbarch_stack_grows_down (gdbarch))
> >>
> >> To make this work for GCS, I had to add another if statement before
> >> the one above, to handle the case where new_ssp is at the initial 0 value:
> >>
> >>   if (gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
> >>       && gdbarch_top_addr_empty_shadow_stack (gdbarch, new_ssp,
> range))
> >>     return {};
> >>   else if (gdbarch_stack_grows_down (gdbarch))
> >>     ⋮
> >>
> >> Otherwise "bt shadow" will print the 0 entry:
> >>
> >> (gdb) bt shadow
> >> #0  0x0000aaaaaaaa08ac in call2 at aarch64-gcs-return.c:65
> >> #1  0x0000aaaaaaaa08c0 in call1 at aarch64-gcs-return.c:71
> >> #2  0x0000aaaaaaaa09d4 in main at aarch64-gcs-return.c:110
> >> #3  0x0000000000000000 in ??
> >
> > Yes, I missed that. Thanks.
> >
> >> > +    {
> >> > +      /* The shadow stack grows downwards.  */
> >> > +      if (new_ssp >= range.second)
> >> > +	{
> >> > +	  /* We reached the bottom of the shadow stack.  */
> >>
> >> In this case, we reached the top actually. This is why I find it
> >> confusing to use bottom/top as if it where agnostic to whether the stack
> grows up or down...
> >
> > In my vision this is to "bottom" actually, if you look at this from a "stack"
> perspective.
> > But I changed this to "We reached the outermost element".
> 
> Thanks.
> 
> >> > +  /* Check if START_SSP points to a shadow stack memory range and
> use
> >> > +     the returned range to determine when to stop unwinding.
> >> > +     Note that a shadow stack memory range can change, due to shadow
> stack
> >> > +     switches for instance on x86 for an inter-privilege far call or when
> >> > +     calling an interrupt/exception handler at a higher privilege level.
> >> > +     Shadow stack for userspace is supported for amd64 linux starting
> with
> >> > +     Linux kernel v6.6.  However, shadow stack switches are not
> supported
> >> > +     due to missing kernel space support.  We therefore implement this
> >> > +     command without support for shadow stack switches for now.
> >> > + */
> >>
> >> Hm, I'm not sure if aarch64-linux supports GCS switching. The
> >> kernel's gcs.rst documentation says:
> >>
> >>   * The architecture provides instructions for switching between guarded
> >>     control stacks with checks to ensure that the new stack is a valid
> >>     target for switching.
> >>
> >> And a comment in AArch64's implementation of the map_shadow_stack
> >> syscall says:
> >>
> >>   /*
> >>    * Put a cap token at the end of the allocated region so it
> >>    * can be switched to.
> >>    */
> >>
> >> But I don't see anything positively mention that it's supported.
> >> I assume so, but I'll have to confirm. If that is the case, later I
> >> will submit a patch with any necessary changes.
> >
> > Ok, so keeping the current comment is fine for you?
> 
> Yes. Sorry, that was another confusing digression from me.

No worries. 😊

> >> > +  std::pair<CORE_ADDR, CORE_ADDR> range;
> >> > +  if (!gdbarch_address_in_shadow_stack_memory_range (gdbarch,
> *start_ssp,
> >> > +						     &range))
> >>
> >> Shouldn't this if condition also check
> >> gdbarch_top_addr_empty_shadow_stack?
> >
> > For x86 this logic is fine, but I assume for GCS this does not work
> > for empty shadow stacks?
> 
> Indeed, we need the extra check for GCS.

The code in backtrace_shadow_command  now looks as follows:

[...]
  /* Check if START_SSP points to a shadow stack memory range and use
     the returned range to determine when to stop unwinding.
     Note that a shadow stack memory range can change, due to shadow stack
     switches for instance on x86 for an inter-privilege far call or when
     calling an interrupt/exception handler at a higher privilege level.
     Shadow stack for userspace is supported for amd64 linux starting with
     Linux kernel v6.6.  However, shadow stack switches are not supported
     due to missing kernel space support.  We therefore implement this
     command without support for shadow stack switches for now.  */
  bool is_top_addr_empty_shadow_stack = false;
  std::pair<CORE_ADDR, CORE_ADDR> range;
  if (!gdbarch_address_in_shadow_stack_memory_range (gdbarch, *start_ssp,
						     &range))
    {
      /* For x86, if the current shadow stack pointer does not point to
	 shadow stack memory but is valid, the shadow stack is empty.  */
      is_top_addr_empty_shadow_stack = true;
    }

  if (!is_top_addr_empty_shadow_stack)
    {
      /* For ARM's Guarded Control Stack, the shadow stack can be empty
	 even though START_SSP points to shadow stack memory range.  */
      is_top_addr_empty_shadow_stack
	= gdbarch_top_addr_empty_shadow_stack_p (gdbarch)
	  && gdbarch_top_addr_empty_shadow_stack (gdbarch, *start_ssp, range);
    }

  if (is_top_addr_empty_shadow_stack)
    {
      gdb_printf (_("The shadow stack is empty.\n"));
      return;
    }
[...]

Is that ok ?

> >> > +    {
> >> > +      /* If the current shadow stack pointer does not point to shadow
> >> > +	 stack memory, the shadow stack is empty.  */
> >> > +      gdb_printf (_("The shadow stack is empty.\n"));
> >> > +      return;
> >> > +    }
> >> > +
> >> > +  /* Extract the first shadow stack frame info (level 0).  */
> >> > +  ssp_unwind_stop_reason reason =
> >> > +ssp_unwind_stop_reason::no_error;
> >> > +  std::optional<shadow_stack_frame_info> current;
> >> > +  CORE_ADDR new_value;
> >> > +  if (read_shadow_stack_memory (gdbarch, *start_ssp, &new_value,
> &reason))
> >> > +    current = {*start_ssp, new_value, 0,
> >> > +	       ssp_unwind_stop_reason::no_error};
> >>
> >> This line fits in 80 columns and doesn't need to be broken.
> >
> > Yes, I changed that.
> >
> >> > +
> >> > +  std::optional<shadow_stack_frame_info> trailing = current;
> >> > +
> >> > +  LONGEST count = -1;
> >> > +  if (current.has_value () && count_exp != nullptr)
> >> > +    {
> >> > +      count = parse_and_eval_long (count_exp);
> >> > +      /* If count is negative, update trailing with the shadow stack frame
> >> > +	 info from which we should start printing.  */
> >>
> >> Actually, trailing will be the frame above the one from which we
> >> should start printing. For example, if count is -3
> >> get_trailing_outermost_shadow_stack_frame_info will return the frame
> >> "which is COUNT elements above the bottom of the shadow stack", which
> >> in this case is the 4th frame counting from the oldest frame. But we
> >> need to start printing from the 3rd oldest frame ...
> >
> >> > +      if (count < 0)
> >> > +	{
> >> > +	  trailing = get_trailing_outermost_shadow_stack_frame_info
> >> > +		       (gdbarch, range, std::abs (count), *current);
> >>
> >> ... so this call should pass std::abs (count) - 1 as argument.
> >
> > Hm, I am not sure if I can follow. With the fix for passing level
> > instead of count my current output looks as follow:
> >
> > ~~~
> > Breakpoint 1, call2 () at /tmp/amd64-shadow-stack.c:21
> > 21        return 42; /* break call2.  */
> > (gdb) bt -past-main
> > #0  call2 () at /tmp /amd64-shadow-stack.c:21
> > #1  0x0000555555555142 in call1 () at /tmp/amd64-shadow-stack.c:27
> > #2  0x0000555555555153 in main () at /tmp/amd64-shadow-stack.c:38
> > #3  0x00007ffff7c2a1ca in __libc_start_call_main ([...]
> > #4  0x00007ffff7c2a28b in __libc_start_main_impl ([...]) at
> > ../csu/libc-start.c:360
> > #5  0x0000555555555065 in _start ()
> > (gdb) bt -past-main -3
> > #3  0x00007ffff7c2a1ca in __libc_start_call_main ([...]) at
> ../sysdeps/nptl/libc_start_call_main.h:58
> > #4  0x00007ffff7c2a28b in __libc_start_main_impl ([...])    at ../csu/libc-
> start.c:360
> > #5  0x0000555555555065 in _start ()
> > (gdb) bt shadow -3
> > #2  0x00007ffff7c2a1ca in __libc_start_call_main at
> > ../sysdeps/nptl/libc_start_call_main.h:74
> > #3  0x00007ffff7c2a28b in __libc_start_main_impl at
> > ../csu/libc-start.c:128
> > #4  0x0000555555555065 in _start
> > (gdb) bt shadow
> > #0  0x0000555555555142 in call1 at /tmp/amd64-shadow-stack.c:28
> > #1  0x0000555555555153 in main at /tmp/amd64-shadow-stack.c:39
> > #2  0x00007ffff7c2a1ca in __libc_start_call_main at
> > ../sysdeps/nptl/libc_start_call_main.h:74
> > #3  0x00007ffff7c2a28b in __libc_start_main_impl at
> > ../csu/libc-start.c:128
> > #4  0x0000555555555065 in _start
> > ~~~
> >
> > Which is correct from my perspective, but maybe I am missing something.
> > I think I lost the overview a bit, there are a number of issues in my
> > code here, unfortunately. :/ Note that for this output I still print the level
> starting at 0. Will apply this change once the other issues are clear.
> 
> I think this is working for you because of the off-by-one error in the
> calculation of level in get_trailing_outermost_shadow_stack_frame_info
> that I mentioned above. I think the off-by-one error here compensates the
> other one.

As I said, in the similar code in stack.c we also don't pass std::abs (count) - 1, but have this
code instead: "trailing = trailing_outermost_frame (-count);"

So I still think we should keep the current calculation and passing of count to
get_trailing_outermost_shadow_stack_frame_info, but I should adapt the
comment.


> >> > +    {
> >> > +      QUIT;
> >> > +
> >> > +      print_shadow_stack_frame_info (gdbarch, print_options, *current,
> >> > +				     LOCATION);
> >> > +
> >> > +      trailing = current;
> >> > +      current = current->unwind_prev_shadow_stack_frame_info
> (gdbarch,
> >> > +							      range);
> >>
> >> This line fits in 80 columns and doesn't need to be broken.
> >
> > Hm, weird, I again count more than 80 columns.
> 
> Strange indeed.
> 
> >> > +    }
> >> > +
> >> > +  /* If we've stopped before the end, mention that.  */  if
> >> > + (current && from_tty)
> >>
> >> While it's correct to use an std::optional in this way to check
> >> whether it has a value, IMHO it's clearer to be more explicit and use the
> has_value method.
> >> It's also consistent with the rest of the code in this function.
> >
> > I agree. I fixed that.
> >
> >> > +    gdb_printf (_("(More shadow stack frames follow...)\n"));
> >> > +
> >> > +  /* If we've run out of shadow stack frames, and the reason appears to
> >> > +     be an error condition, print it.  */  if (!current.has_value
> >> > + () && trailing.has_value ()
> >>
> >> If I'm not mistaken, trailing.has_value () is always true at this point.
> >
> > I agree, this logic is from backtrace_command_1.
> > I think adding an assert here should be fine instead, so I suggest the
> following:
> >
> > ~~~
> > [...]
> >   /* If we've stopped before the end, mention that.  */
> >   if (current.has_value () && from_tty)
> >     gdb_printf (_("(More shadow stack frames follow...)\n"));
> >
> >   /* Due to the loop above, trailing always has a value at this point.  */
> >   gdb_assert (trailing.has_value ());
> >
> >   /* If we've run out of shadow stack frames, and the reason appears to
> >      be an error condition, print it.  */ [...] ~~~
> >
> > Please let me know if you think otherwise.
> 
> Looks good to me.
> 
> --
> Thiago

Thanks again for the feedback,

Christina
Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928