RE: [PATCH v5 00/12] Add CET shadow stack support

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

Hi Luis,

Thank you for the feedback.

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Monday, July 14, 2025 9:13 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: thiago.bauermann@linaro.org
> Subject: Re: [PATCH v5 00/12] Add CET shadow stack support
> 
> On 7/13/25 20:05, Luis Machado wrote:
> > Hi,
> >
> > On 7/13/25 15:01, Schimpe, Christina wrote:
> >> Hi Luis,
> >>
> >> Thanks for sharing that info.
> >> I tried to reproduces with the same ubuntu & kernel versions and a very
> similar system but am not successful so far...
> >> Just to be sure, I assume you test with gcc compiler (which version?) and
> compiled GDB with latest upstream master + my cet shadow stack patches?
> >
> > Interesting. GCC says: gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0
> >
> > I applied the CET shadow stack patches from v5. And yes, gdb is the latest
> tip-of-tree.
> >>
> >> Do you see this for target_board=native-gdbserver and
> target_board=unix?
> >
> > No. native-gdbserver, native-extended-gdbserver and unix work just fine.
> >
> > Let me have another look to see what might be going on.
> 
> Heh, found it. I had a freshly-installed system and was missing libexpat-dev,
> so no XML was being generated.
> 
> With libexpat-dev installed, everything looks fine. So this is good. Sorry for
> the noise.

No worries!

> I spotted two things while building gdb with the patch series applied
> 
> There is a uninitialized usage warning
> 
> gdb/nat/x86-linux.c:151:12: warning: 'ecx' may be used uninitialized [-
> Wmaybe-uninitialized]
>   151 |   if ((ecx & bit_SHSTK) == 0)
>       |       ~~~~~^~~~~~~~~~~~
> ../../../repos/binutils-gdb/gdb/nat/x86-linux.c:147:26: note: 'ecx' was declared
> here
>   147 |   unsigned int eax, ebx, ecx, edx;

Yes, thank you for catching this. I'll fix it as follows:

diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c
index 1756d5441fc..d037992d849 100644
--- a/gdb/nat/x86-linux.c
+++ b/gdb/nat/x86-linux.c
@@ -145,6 +145,7 @@ x86_check_ssp_support (const int tid)
      stack is not enabled for the current thread, we still want to return
      true.  */
   unsigned int eax, ebx, ecx, edx;
+  eax = ebx = ecx = edx = 0;

> Also, git am seems to complain about some whitespace issues:
> Applying: gdb, gdbserver: Add support of Intel shadow stack pointer register.
> .git/rebase-apply/patch:596: indent with spaces.
>             # Having unavailable registers leads to a fall back to the standard
> .git/rebase-apply/patch:597: indent with spaces.
>             # unwinders.  Don't add unavailable registers to avoid this.
> .git/rebase-apply/patch:598: indent with spaces.
>             if (str (val) == "<unavailable>"):
> .git/rebase-apply/patch:599: indent with spaces.
>                 continue

I would assume that these findings are false positives, since it's a .py file and the
idents are consistently with spaces in that file.
Is that ok?

> .git/rebase-apply/patch:587: new blank line at EOF.
> +

Thanks! Will fix.

I also spotted a few whitespace issues through the series: 

diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 782b66f1467..a2b49730e00 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1981,7 +1981,7 @@ amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR new_addr,
     return;
 
   /* The shadow stack grows downwards.  To push addresses to the stack,
-     we need to decrement SSP.    */
+     we need to decrement SSP.  */
   const int element_size
     = amd64_linux_shadow_stack_element_size_aligned (gdbarch);
   const CORE_ADDR new_ssp = *ssp - element_size;
@@ -2004,7 +2004,7 @@ amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR new_addr,
   regcache_raw_write_unsigned (regcache, tdep->ssp_regnum, new_ssp);
 }
 
-/* Implement shadow stack pointer unwinding. For each new shadow stack
+/* Implement shadow stack pointer unwinding.  For each new shadow stack
    pointer check if its address is still in the shadow stack memory range.
    If it's outside the range set the returned value to unavailable,
    otherwise return a value containing the new shadow stack pointer.  */
diff --git a/gdb/arch/x86-linux-tdesc-features.h b/gdb/arch/x86-linux-tdesc-features.h
index d1d74e70df8..a280bf608d7 100644
--- a/gdb/arch/x86-linux-tdesc-features.h
+++ b/gdb/arch/x86-linux-tdesc-features.h
@@ -58,7 +58,7 @@ extern int x86_linux_i386_tdesc_count ();
    x86_linux_xstate_bv_mask_to_tdesc_idx) into an xstate_bv_mask
    value which can then be used to create a target description.
    The return mask has the same format than the state component bitmap
-   and does include user and supervisor state components.*/
+   and does include user and supervisor state components.  */
 
 extern uint64_t x86_linux_tdesc_idx_to_xstate_bv_mask (int idx);
 
diff --git a/gdb/testsuite/gdb.arch/amd64-ssp.exp b/gdb/testsuite/gdb.arch/amd64-ssp.exp
index 6ddc875b9a3..18d43065034 100644
--- a/gdb/testsuite/gdb.arch/amd64-ssp.exp
+++ b/gdb/testsuite/gdb.arch/amd64-ssp.exp
@@ -47,4 +47,3 @@ save_vars { ::env(GLIBC_TUNABLES) } {
     # configuring the shadow stack pointer.
     gdb_continue_to_end
 }
-
diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
index 1a0393d127c..beb04b7f8f6 100644
--- a/gdbserver/linux-i386-ipa.cc
+++ b/gdbserver/linux-i386-ipa.cc
@@ -174,7 +174,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
 const struct target_desc *
 get_ipa_tdesc (int idx)
 {
-  uint64_t xstate_bv_mask = x86_linux_tdesc_idx_to_xstate_bv_mask  (idx);
+  uint64_t xstate_bv_mask = x86_linux_tdesc_idx_to_xstate_bv_mask (idx); 

> warning: 5 lines add whitespace errors.

which I plan to fix locally, too.

> 
> Could you please address those locally?
> 
> Otherwise...
> 
> Approved-By: Luis Machado <luis.machado@arm.com>
> 
> I'd give this another week in case someone has any more comments, and
> then push it.
> 
> Thanks for the series.

My plan is to address your feedback as described here and also Andrew's feedback for the v5.
So far I haven't received any feedback that would block merging this or requires a new version think,
but my plan would be to wait until middle next week. I also plan to include parts of Thiago's patch
"gdb, testsuite: Extend core_find procedure to save program output" (first patch of v5), if his series
will not be merged before mine.

Please let me know if you have any concerns with that plan.

Thanks,
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928