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

[Guinevere Larsen <guinevere@redhat.com>]

On 12/20/24 5:05 PM, Schimpe, Christina wrote:
> This patch is required by the following commit.
> ---

Maybe others disagree (in which case go with their suggestion), but my 
opinion is that, because you're only adding gdbarch functionality but no 
one uses it yet, there's basically no chance this commit could 
inadvertently break something, so it is fine to merge with the next one 
where you make use of the feature.

If these are split, I would like to have some explanation as to why it 
is required, but I imagine the explanation finds a better home in the 
next commit anyway, which is another reason why I'd join both commits.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>   gdb/arch-utils.c          |  8 ++++++++
>   gdb/arch-utils.h          |  5 +++++
>   gdb/gdbarch-gen.c         | 22 ++++++++++++++++++++++
>   gdb/gdbarch-gen.h         |  6 ++++++
>   gdb/gdbarch_components.py | 10 ++++++++++
>   5 files changed, 51 insertions(+)
>
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index a2ed2a2c011..3aedb3f2600 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1218,6 +1218,14 @@ default_gdbarch_return_value
>   				readbuf, writebuf);
>   }
>   
> +/* See arch-utils.h.  */
> +
> +std::optional<CORE_ADDR>
> +default_get_shadow_stack_pointer (gdbarch *gdbarch)
> +{
> +  return {};
> +}
> +
>   obstack *gdbarch_obstack (gdbarch *arch)
>   {
>     return &arch->obstack;
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index b59e2ad70c2..471be9c282d 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -325,4 +325,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);
> +
>   #endif /* GDB_ARCH_UTILS_H */
> diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c
> index c33c476dfdb..72638f01ad7 100644
> --- a/gdb/gdbarch-gen.c
> +++ b/gdb/gdbarch-gen.c
> @@ -261,6 +261,7 @@ struct gdbarch
>     gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
>     gdbarch_use_target_description_from_corefile_notes_ftype *use_target_description_from_corefile_notes = default_use_target_description_from_corefile_notes;
>     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
> @@ -533,6 +534,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>     /* Skip verify of read_core_file_mappings, invalid_p == 0.  */
>     /* Skip verify of use_target_description_from_corefile_notes, 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 ());
> @@ -1404,6 +1406,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);
>   }
> @@ -5539,3 +5544,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)
> +{
> +  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);
> +}
> +
> +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 ff14a3666b9..ff511b4a9e0 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1783,6 +1783,8 @@ extern void set_gdbarch_use_target_description_from_corefile_notes (struct gdbar
>      technologies.  For example, Intel's Control-flow Enforcement Technology (CET)
>      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. */
> @@ -1792,3 +1794,7 @@ extern bool gdbarch_shadow_stack_push_p (struct gdbarch *gdbarch);
>   typedef void (gdbarch_shadow_stack_push_ftype) (struct gdbarch *gdbarch, CORE_ADDR new_addr);
>   extern void gdbarch_shadow_stack_push (struct gdbarch *gdbarch, CORE_ADDR new_addr);
>   extern void set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch, gdbarch_shadow_stack_push_ftype *shadow_stack_push);
> +
> +typedef std::optional<CORE_ADDR> (gdbarch_get_shadow_stack_pointer_ftype) (struct gdbarch *gdbarch);
> +extern std::optional<CORE_ADDR> gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch);
> +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 52f265e8e0e..df70cb082a4 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -2822,6 +2822,8 @@ Some targets support special hardware-assisted control-flow protection
>   technologies.  For example, Intel's Control-flow Enforcement Technology (CET)
>   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.
> @@ -2831,3 +2833,11 @@ pointer.
>       params=[("CORE_ADDR", "new_addr")],
>       predicate=True,
>   )
> +
> +Method(
> +    type="std::optional<CORE_ADDR>",
> +    name="get_shadow_stack_pointer",
> +    params=[],
> +    predefault="default_get_shadow_stack_pointer",
> +    invalid=False,
> +)