Re: [PATCH v2 6/9] gdb: Add command option 'bt -shadow' to print the shadow stack backtrace.

[Tom Tromey <tom@tromey.com>]

>>>>> Christina Schimpe <christina.schimpe@intel.com> writes:

> Add command option '-shadow" to the backtrace command to print the shadow
> stack backtrace instead of the normal backtrace.

Thanks.

> -annotate_frame_function_name (void)
> +annotate_frame_function_name (bool shadowstack_frame)
>  {
>    if (annotation_level == 2)
> -    printf_unfiltered (("\n\032\032frame-function-name\n"));
> +    {
> +      if (!shadowstack_frame)
> +	printf_unfiltered (("\n\032\032frame-function-name\n"));
> +      else
> +	printf_unfiltered (("\n\032\032shadow-stack-frame-function-name\n"));
> +    }

I think it is fine to just drop all the annotation changes.
As far as I know, no client even uses annotation_level > 1.

Emacs, maybe the only existing user of annotations, passes --fullname
which uses:

	  case 'f':
	    annotation_level = 1;

> +  if (should_print_location (print_what) || sal.symtab == nullptr)
> +    {
> +      gdb::unique_xmalloc_ptr<char> funname = find_pc_funname (frame.value);
> +
> +      { /* Extra scope to print frame tuple.  */
> +	ui_out_emit_tuple tuple_emitter (uiout, "shadow-stack-frame");

The extra scope doesn't really look necessary here, since:

> +      } /* Extra scope to print frame tuple.  */
> +
> +      uiout->text ("\n");

... it seems fine to emit this text before the tuple emitter is
destroyed?  And there's already an extras scope from the "then" block.

> +  gdb_flush (gdb_stdout);

Is flushing really needed?  I feel like I saw this in a different patch
in the series as well, and forgot to ask about it there.

> +
> +/* Read the memory at shadow stack pointer SSP and assign it to
> +   RETURN_VALUE.  In case we cannot read the memory, set REASON to
> +   ssp_unwind_stop_reason::memory_read_error and return false.  */

It seems odd to combine a bool return and an out parameter when the enum
has a "no_error" value.

Maybe just a bool return would be more appropriate and then the callers
could set their own out parameters.  Especially since one caller doesn't
even need this.

> -static const char *const print_frame_info_choices[] =
> +const char *const print_frame_info_choices[] =

This is public now but I didn't see other uses of it.

Tom