RE: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method to get the shadow stack pointer.

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

Hi Andrew,

Thanks a lot for the feedback.

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Thursday, August 14, 2025 5:51 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com
> Subject: RE: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method to get
> the shadow stack pointer.
> 
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> 
> > Hi Andrew,
> >
> > Thanks for the feedback. Please find my comments to your feedback below.
> >
> >> -----Original Message-----
> >> From: Andrew Burgess <aburgess@redhat.com>
> >> Sent: Wednesday, July 30, 2025 2:22 PM
> >> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> >> patches@sourceware.org
> >> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com
> >> Subject: Re: [PATCH v5 11/12] gdb, gdbarch: Introduce gdbarch method
> >> to get the shadow stack pointer.
> >>
> >> Christina Schimpe <christina.schimpe@intel.com> writes:
> >>
> >> > This patch is required by the following commit
> >> > "gdb: Enable displaced stepping with shadow stack on amd64 linux."
> >> >
> >> > Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> >> > Reviewed-By: Luis Machado <luis.machado@arm.com>
> >> > ---
> >> >  gdb/arch-utils.c          | 10 ++++++++++
> >> >  gdb/arch-utils.h          |  5 +++++
> >> >  gdb/gdbarch-gen.c         | 22 ++++++++++++++++++++++
> >> >  gdb/gdbarch-gen.h         | 12 +++++++++++-
> >> >  gdb/gdbarch_components.py | 17 ++++++++++++++++-
> >> >  5 files changed, 64 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index
> >> > f320d3d7365..c396e9e3840 100644
> >> > --- a/gdb/arch-utils.c
> >> > +++ b/gdb/arch-utils.c
> >> > @@ -1218,6 +1218,16 @@ default_gdbarch_return_value
> >> >  				readbuf, writebuf);
> >> >  }
> >> >
> >> > +/* See arch-utils.h.  */
> >> > +
> >> > +std::optional<CORE_ADDR>
> >> > +default_get_shadow_stack_pointer (gdbarch *gdbarch, regcache
> >> *regcache,
> >> > +				  bool &shadow_stack_enabled)
> >> > +{
> >> > +  shadow_stack_enabled = false;
> >> > +  return {};
> >> > +}
> >> > +
> >> >  obstack *gdbarch_obstack (gdbarch *arch)  {
> >> >    return &arch->obstack;
> >> > diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h index
> >> > 1509cb7441e..14a84b74733 100644
> >> > --- a/gdb/arch-utils.h
> >> > +++ b/gdb/arch-utils.h
> >> > @@ -414,4 +414,9 @@ extern enum return_value_convention
> >> default_gdbarch_return_value
> >> >        struct regcache *regcache, struct value **read_value,
> >> >        const gdb_byte *writebuf);
> >> >
> >> > +/* Default implementation of gdbarch default_get_shadow_stack_pointer
> >> > +   method.  */
> >> > +extern std::optional<CORE_ADDR> default_get_shadow_stack_pointer
> >> > +  (gdbarch *gdbarch, regcache *regcache, bool
> >> > +&shadow_stack_enabled);
> >> > +
> >> >  #endif /* GDB_ARCH_UTILS_H */
> >> > diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c index
> >> > a4b72793fd8..caeda3cefae 100644
> >> > --- a/gdb/gdbarch-gen.c
> >> > +++ b/gdb/gdbarch-gen.c
> >> > @@ -263,6 +263,7 @@ struct gdbarch
> >> >    gdbarch_use_target_description_from_corefile_notes_ftype
> >> *use_target_description_from_corefile_notes =
> >> default_use_target_description_from_corefile_notes;
> >> >    gdbarch_core_parse_exec_context_ftype *core_parse_exec_context =
> >> default_core_parse_exec_context;
> >> >    gdbarch_shadow_stack_push_ftype *shadow_stack_push = nullptr;
> >> > +  gdbarch_get_shadow_stack_pointer_ftype *get_shadow_stack_pointer
> >> > + = default_get_shadow_stack_pointer;
> >> >  };
> >> >
> >> >  /* Create a new ``struct gdbarch'' based on information provided
> >> > by @@ -537,6 +538,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
> >> >    /* Skip verify of use_target_description_from_corefile_notes,
> >> > invalid_p
> >> == 0.  */
> >> >    /* Skip verify of core_parse_exec_context, invalid_p == 0.  */
> >> >    /* Skip verify of shadow_stack_push, has predicate.  */
> >> > +  /* Skip verify of get_shadow_stack_pointer, invalid_p == 0.  */
> >> >    if (!log.empty ())
> >> >      internal_error (_("verify_gdbarch: the following are invalid ...%s"),
> >> >  		    log.c_str ());
> >> > @@ -1414,6 +1416,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct
> >> ui_file *file)
> >> >    gdb_printf (file,
> >> >  	      "gdbarch_dump: shadow_stack_push = <%s>\n",
> >> >  	      host_address_to_string (gdbarch->shadow_stack_push));
> >> > +  gdb_printf (file,
> >> > +	      "gdbarch_dump: get_shadow_stack_pointer = <%s>\n",
> >> > +	      host_address_to_string
> >> > +(gdbarch->get_shadow_stack_pointer));
> >> >    if (gdbarch->dump_tdep != NULL)
> >> >      gdbarch->dump_tdep (gdbarch, file);  } @@ -5583,3 +5588,20 @@
> >> > set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch,  {
> >> >    gdbarch->shadow_stack_push = shadow_stack_push;  }
> >> > +
> >> > +std::optional<CORE_ADDR>
> >> > +gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch,
> >> > +regcache *regcache, bool &shadow_stack_enabled) {
> >> > +  gdb_assert (gdbarch != NULL);
> >> > +  gdb_assert (gdbarch->get_shadow_stack_pointer != NULL);
> >> > +  if (gdbarch_debug >= 2)
> >> > +    gdb_printf (gdb_stdlog, "gdbarch_get_shadow_stack_pointer
> >> > +called\n");
> >> > +  return gdbarch->get_shadow_stack_pointer (gdbarch, regcache,
> >> > +shadow_stack_enabled); }
> >> > +
> >> > +void
> >> > +set_gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch,
> >> > +
> >> gdbarch_get_shadow_stack_pointer_ftype
> >> > +get_shadow_stack_pointer) {
> >> > +  gdbarch->get_shadow_stack_pointer = get_shadow_stack_pointer; }
> >> > diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index
> >> > 71142332540..c36171b089e 100644
> >> > --- a/gdb/gdbarch-gen.h
> >> > +++ b/gdb/gdbarch-gen.h
> >> > @@ -1807,7 +1807,8 @@ extern void
> >> set_gdbarch_core_parse_exec_context (struct gdbarch *gdbarch, gdbarc
> >> >     technologies.  For example, the Intel Control-Flow Enforcement
> >> Technology
> >> >     (Intel CET) on x86 provides a shadow stack and indirect branch tracking.
> >> >     To enable shadow stack support for inferior calls the
> >> shadow_stack_push
> >> > -   gdbarch hook has to be provided.
> >> > +   gdbarch hook has to be provided.  The get_shadow_stack_pointer
> >> gdbarch
> >> > +   hook has to be provided to enable displaced stepping.
> >> >
> >> >     Push NEW_ADDR to the shadow stack and update the shadow stack
> >> > pointer. */
> >> >
> >> > @@ -1816,3 +1817,12 @@ extern bool gdbarch_shadow_stack_push_p
> >> (struct
> >> > gdbarch *gdbarch);  typedef void (gdbarch_shadow_stack_push_ftype)
> >> > (struct gdbarch *gdbarch, CORE_ADDR new_addr, regcache *regcache);
> >> > extern void gdbarch_shadow_stack_push (struct gdbarch *gdbarch,
> >> > CORE_ADDR new_addr, regcache *regcache);  extern void
> >> > set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch,
> >> > gdbarch_shadow_stack_push_ftype *shadow_stack_push);
> >> > +
> >> > +/* If possible, return the shadow stack pointer.  On some
> >> > +architectures,
> >> the
> >> > +   shadow stack pointer is available even if the feature is disabled.  To
> >> > +   return the feature's enablement state configure
> >> SHADOW_STACK_ENABLED.
> >> > +   Set it to true in case the shadow stack is enabled. */
> >> > +
> >> > +typedef std::optional<CORE_ADDR>
> >> > +(gdbarch_get_shadow_stack_pointer_ftype) (struct gdbarch *gdbarch,
> >> > +regcache *regcache, bool &shadow_stack_enabled); extern
> >> > +std::optional<CORE_ADDR> gdbarch_get_shadow_stack_pointer (struct
> >> > +gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled);
> >> > +extern void set_gdbarch_get_shadow_stack_pointer (struct gdbarch
> >> > +*gdbarch, gdbarch_get_shadow_stack_pointer_ftype
> >> > +*get_shadow_stack_pointer);
> >> > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> >> > index abc79588473..73459064170 100644
> >> > --- a/gdb/gdbarch_components.py
> >> > +++ b/gdb/gdbarch_components.py
> >> > @@ -2855,7 +2855,8 @@ Some targets support special hardware-
> >> assisted
> >> > control-flow protection  technologies.  For example, the Intel
> >> > Control-Flow Enforcement Technology  (Intel CET) on x86 provides a
> >> shadow stack and indirect branch tracking.
> >> >  To enable shadow stack support for inferior calls the
> >> > shadow_stack_push -gdbarch hook has to be provided.
> >> > +gdbarch hook has to be provided.  The get_shadow_stack_pointer
> >> > +gdbarch hook has to be provided to enable displaced stepping.
> >>
> >> I find the addition of this last sentence here a little strange.
> >> While it's a true statement, wouldn't this be better placed on the
> >> comment for get_shadow_stack_pointer?
> >
> > Mh I see the initial comment section here rather as general overview
> > for the shadow stack feature in GDB. It explains which features (e.g.
> > infcalls and displaced
> > stepping) are interacting with shadow stacks.  So in my opinion it's okay to
> keep it as is.
> >
> >> >
> >> >  Push NEW_ADDR to the shadow stack and update the shadow stack
> >> pointer.
> >> >  """,
> >> > @@ -2864,3 +2865,17 @@ Push NEW_ADDR to the shadow stack and
> >> update the shadow stack pointer.
> >> >      params=[("CORE_ADDR", "new_addr"), ("regcache *", "regcache")],
> >> >      predicate=True,
> >> >  )
> >> > +
> >> > +Method(
> >> > +    comment="""
> >> > +If possible, return the shadow stack pointer.  On some
> >> > +architectures, the shadow stack pointer is available even if the
> >> > +feature is disabled.  To return the feature's enablement state
> >> > +configure
> >> SHADOW_STACK_ENABLED.
> >> > +Set it to true in case the shadow stack is enabled.
> >>
> >> The wording "configure SHADOW_STACK_ENABLED" seems a little strange.
> >> Also, there's a bunch of important detail that this comment doesn't cover.
> >> Here's what I'd suggest, though it's possible this doesn't match the
> >> implementation (I haven't checked the next patch yet), but this does
> >> match default_get_shadow_stack_pointer.  Feel free to take any of
> >> this that is
> >> useful:
> >>
> >>   If possible, return the shadow stack pointer.  On some architectures,
> >>   the shadow stack pointer is available even if the feature is disabled.
> >>   If the shadow stack feature is enabled then set SHADOW_STACK_ENABLED
> >>   to true, otherwise set SHADOW_STACK_ENABLED to false.  The
> >>   SHADOW_STACK_ENABLED will always be set if this function returns a
> >>   value.  If the function doesn't return a value then the state of
> >>   SHADOW_STACK_ENABLED is undefined.
> >
> > Hm, I am not sure if I am missing something here.
> >
> > We set SHADOW_STACK_ENABLED to false in
> > default_get_shadow_stack_pointer and *do not*  return a value.
> > ~~~
> > std::optional<CORE_ADDR>
> > default_get_shadow_stack_pointer (gdbarch *gdbarch, regcache *regcache,
> > 				  bool &shadow_stack_enabled)
> > {
> >   shadow_stack_enabled = false;
> >   return {};
> > }
> > ~~~
> >
> > What do you think about the following:
> > If possible, return the shadow stack pointer.  If the shadow stack
> > feature is enabled then set SHADOW_STACK_ENABLED to true, otherwise
> > set SHADOW_STACK_ENABLED to false.
> > On some architectures, the shadow stack pointer is available even if the feature
> is disabled.
> > So dependent on the target, an implementation of this function may
> > return a valid shadow stack pointer, but set  SHADOW_STACK_ENABLED to
> false.
> 
> That's great.  Could you also add a sentence similar to the one added to
> shadow_stack_push that get_shadow_stack_pointer must be implemented in
> order for displaced stepping to work.
> 
> I still don't understand why you want to document that detail in the comment
> for shadow_stack_push, but I don't think it's going to do any harm.  But I do
> think details about get_shadow_stack_pointer should be documented on
> get_shadow_stack_pointer!

I totally agree, otherwise there is the risk that this is overseen.

I'll change it to:
If possible, return the shadow stack pointer.  If the shadow stack 
feature is enabled then set SHADOW_STACK_ENABLED to true, otherwise 
set SHADOW_STACK_ENABLED to false.  This hook has to be provided to enable
displaced stepping for shadow stack enabled programs.
On some architectures, the shadow stack pointer is available even if the
feature is disabled.  So dependent on the target, an implementation of
this function may return a valid shadow stack pointer, but set
SHADOW_STACK_ENABLED to false.

> 
> With that extra sentence added:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew

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