RE: [PATCH v2 0/9] Add new command to print the shadow stack backtrace

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

Hi Tom and Thiago, 

First, thanks a lot for your review efforts on this series so far!
As indicated in the cover letter of v2, I would like to post v3, including the -past-main option.

Enabling this made me realize that I need some refactoring of the commits:
- "gdb: Add command option 'bt -shadow' to print the shadow stack backtrace."
- "gdb: Provide gdbarch hook to distinguish shadow stack backtrace elements."

I have mixed feelings about this. On the one hand, the patches have already been very well
reviewed (thanks again!) and the refactoring means that new review effort is required. But
on the other hand, I think these changes improve the series overall. Below, I briefly describe
the main changes to those commits. If this approach makes sense to you, I will post v3
shortly after.

To avoid increasing your review effort, I also considered adding these changes as new
patches on top. However, it is probably cleaner to merge them into the existing commits.
Another option is that I post the V3 including fixups on top (and merge in a v4) to make
the review easier for you. Or, we could also consider merging the series (once fully approved)
without -past-main and I could submit a follow-up series for that feature. If you have any
preference, I'm happy about your feedback.

My main changes to enable -past-main are:

1) A new commit (required before "gdb: Add command option 'bt -shadow' to print the shadow stack backtrace."): 

    gdb: Add get_main_func_start_pc to refactor frame.c:inside_main_func.

    Refactor frame.c:inside_main_func to use the new function
    get_main_func_start_pc, which will be used in a following commit.

2) Refactoring of "gdb: Add command option 'bt -shadow' to print the shadow stack backtrace." :

    To enable the -past-main option in the shadow stack backtrace the function
    get_main_func_start_pc (gdbarch* gdbarch, const language lang) is called by a
    new function shadow_stack_frame_info::inside_main_func.  This probably requires
    a frame specific gdbarch and language in the struct shadow_stack_frame_info.
    I am not sure the gdbarch or language ever changes for IA, but it might be a good
    idea to add it to be safe. So, due to that we need some refactoring of the shadow
    stack backtrace implementation:
    a. The frame specific arch is extracted based on the symtab_and_line (SAL)
       object.  To avoid that we extract the SAL and gdbarch multiple times for
       the same frame we store it in the shadow stack frame.
    b. The gdbarch hook get_shadow_stack_size is obsolete since the function
       get_trailing_outermost_shadow_stack_frame_info now unwinds each frame.  This
       helps to configure the previous frame attributes in case the gdbarch or SAL
       information cannot be extracted for special shadow stack elements.  We don't need
       the COUNT parameter anymore in the function update_shadow_stack_pointer, too.
       Now, this is also inline with the function trailing_outermost_frame which is used
       for the normal backtrace.
    c. Introduce function get_shadow_stack_frame_info to share some more logic:
    ~~~
    /* If possible, get shadow stack frame info for the shadow stack pointer
       SSP and its current frame LEVEL.  Pass a fallback gdbarch FALLBACK_ARCH
       which can be used as fallback in case the gdbarch cannot be extracted
       from the SAL.  Usually this is the gdbarch of the previous frame.  */

    static std::optional<shadow_stack_frame_info>
    get_shadow_stack_frame_info
      (gdbarch *fallback_arch, const CORE_ADDR ssp, ULONGEST level)
    ~~~

3) Refactoring of "gdb: Provide gdbarch hook to distinguish shadow stack backtrace elements.

    a. Like normal frames used in the ordinary backtrace command, introduce an enum
       ssp_frame_type (normal_frame, non_return_frame) which classifies each frame and is
       an attribute of the shadow stack frame info.  Calling of inside_main_func () to check
       if we should stop unwinding is only done for normal frames.  For the currently known
       cases of non-return-address shadow stack elements, the value is not a PC of the program
       and the past-main check is not applicable.
       More frame types will be added in the following patches to enable sigtramp and inferior
       call frames.
    b. The gdbarch hook is_no_return_shadow_stack_address now returns the complete shadow stack
       frame (classified as non_return_frame) in case the element on the shadow stack is not a return
       address (instead of the optional return address description string used before). The target can
       then assign the correct gdbarch and an optional SAL object. Due to that hook is no longer called 
       when printing the frame, but already when constructing it in get_shadow_stack_frame_info and
       we don't pass the shadow stack frame as argument anymore, but the shadow stack pointer, its
       value and level.

       The new interface looks as follows:
    ~~~
    Method(
        comment="""
    There can be elements on the shadow stack which are not return addresses.
    This happens for example on x86 with CET in case of signals.
    If an architecture implements the command options 'backtrace -shadow' and
    the shadow stack can contain elements which are not return addresses, this
    function has to be provided.
    Return a shadow stack frame info with frame type
    ssp_frame_type::non_return_frame, if the shadow stack pointer SSP belongs
    to a valid shadow stack frame while the element on the shadow stack
    VALUE does not refer to a return address.  In that case, also the frame's
    attribute non_return_description must be set to a string which is displayed
    instead of the element on the shadow stack in the shadow stack backtrace.
    Otherwise, return an empty optional.
    """,
        type="std::optional<shadow_stack_frame_info>",
        name="is_no_return_shadow_stack_address",
        params=[
          ("const CORE_ADDR", "ssp"),
          ("const CORE_ADDR", "value"),
          ("const unsigned long", "level")
        ],
        predicate=True,
    )
    ~~~
    c. The shadow stack frame contains a new string attribute "non_return_description"
        which is printed instead of the element on the shadow stack in case the frame is
        classified as non-return address frame.  It is empty in case the frame is not a
       " ssp_frame_type::non_return_frame" frame.  We could also use an optional here,
       I am not sure what is preferred.

This is now the class for a shadow stack frame:

/* Information of a shadow stack frame belonging to a shadow stack element
   at shadow stack pointer SSP.  */

class shadow_stack_frame_info
{
public:
  /* If possible, unwind the previous shadow stack frame info.  RANGE is
     the shadow stack memory range [start_address, end_address) belonging
     to this frame's shadow stack pointer.  If we cannot unwind the
     previous frame info, set the unwind_stop_reason attribute.  If we
     reached the bottom of the shadow stack just don't return a value.  */
  std::optional<shadow_stack_frame_info> unwind_prev_shadow_stack_frame_info
    (std::pair<CORE_ADDR, CORE_ADDR> range);

  /* The shadow stack pointer.  */
  CORE_ADDR ssp;

  /* The value of the shadow stack at SSP.  */
  CORE_ADDR value;

  /* The level of the element on the shadow stack.  */
  unsigned long level;

  /* Architecture of the current shadow stack frame.  */
  struct gdbarch *arch;

  /* Optional SAL object of the current shadow stack frame.  */
  std::optional<symtab_and_line> sal;

  /* The shadow stack frame type.  */
  ssp_frame_type type;

  /* If this is not a normal shadow stack frame belonging to a return
     address of the program execution flow then the frame will have the
     type ssp_frame_type::non_return_frame.  In this case, this string
     is configured to describe this specific frame type and it will
     be printed in the shadow stack backtrace instead of the element on
     the shadow stack.  */
  std::string non_return_description;

  /* If unwinding of the previous frame info fails assign this value to a
     matching condition ssp_unwind_stop_reason
     > ssp_unwind_stop_reason::no_error.  */
  ssp_unwind_stop_reason unwind_stop_reason
    = ssp_unwind_stop_reason::no_error;

  /* Return true if this frame is inside the "main"() function.  If the frame
     is outside the "main"() function, does not have a SAL object or if the
     symbol information cannot be extracted, return false.  */
  bool inside_main_func () const;
};

I am looking forward to your feedback!

Kind Regards,
Christina
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 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 Muenchen HRB 186928