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

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

Hi Luis, 

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Thursday, June 19, 2025 11:26 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; eliz@gnu.org
> Subject: Re: [PATCH v4 10/11] gdb, gdbarch: Introduce gdbarch method to get
> the shadow stack pointer.
> 
> On 6/17/25 13:11, Christina Schimpe wrote:
> > This patch is required by the following commit.
> 
> What particular entry in the series uses this? The displaced stepping one?
> 
> If so, we should make it more explicit.
> 
> >
> > Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> > ---
> >  gdb/arch-utils.c          | 10 ++++++++++
> >  gdb/arch-utils.h          |  5 +++++
> >  gdb/gdbarch-gen.c         | 22 ++++++++++++++++++++++
> >  gdb/gdbarch-gen.h         | 10 ++++++++++
> >  gdb/gdbarch_components.py | 15 +++++++++++++++
> >  5 files changed, 62 insertions(+)
> >
> > diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index
> > be0494fba1e..c86df72a6d8 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
> > 3ca19c427a3..4490d53a940 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
> > c276cd66d71..b8d1df94f59 100644
> > --- a/gdb/gdbarch-gen.h
> > +++ b/gdb/gdbarch-gen.h
> > @@ -1806,6 +1806,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 inferior calls the function shadow_stack_push has to be provided.
> > +   The method get_shadow_stack_pointer has to be provided to enable
> displaced
> > +   stepping.
> >
> >     Push the address NEW_ADDR on the shadow stack and update the shadow
> stack
> >     pointer. */
> > @@ -1815,3 +1817,11 @@ 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 shadow stack enablement state configure
> > +SHADOW_STACK_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 ab685b14ec7..ccbdcaf07c9 100644
> > --- a/gdb/gdbarch_components.py
> > +++ b/gdb/gdbarch_components.py
> > @@ -2855,6 +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 inferior calls the function shadow_stack_push has to be provided.
> > +The method get_shadow_stack_pointer has to be provided to enable
> > +displaced
> 
> Maybe...
> 
> "The get_shadow_stack_pointer gdbarch hook has to be..."
> 
> > +stepping.
> >
> >  Push the address NEW_ADDR on the shadow stack and update the shadow
> > stack  pointer.
> > @@ -2864,3 +2866,16 @@ 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 shadow stack enablement state configure
> SHADOW_STACK_ENABLED.
> > +""",
> > +    type="std::optional<CORE_ADDR>",
> > +    name="get_shadow_stack_pointer",
> > +    params=[("regcache *", "regcache"), ("bool &", "shadow_stack_enabled")],
> > +    predefault="default_get_shadow_stack_pointer",
> > +    invalid=False,
> > +)
> 
> Otherwise looks OK.
> 
> Reviewed-By: Luis Machado <luis.machado@arm.com>

Thanks for the review. For v5, I will apply all your comments and also fix the comment
as suggested to Tom here, if there are no concerns:
https://sourceware.org/pipermail/gdb-patches/2025-June/218864.html

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