RE: [PATCH v4 09/11] gdb: Implement amd64 linux shadow stack support for inferior calls.

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

Hi Luis, 

Thanks for the review.

<...>

> > +
> > +  /* Starting with v6.6., the Linux kernel supports CET shadow stack.
> 
> Typo, period after "v6.6".
> 
> > +     Dependent on the target the ssp register can be invalid or nullptr
> > +     when shadow stack is supported by HW and the linux kernel but not
> > +     enabled for the current thread.  */
> 
> I feel the comment doesn't quite reflect this case very well. We don't have a
> nullptr here, so I suppose ssp == 0x0 means ssp is unavailable? We should
> make that clear here, but we don't need to have this more generic comment
> pasted here again, as it was explained elsewhere already.

I agree. I removed the "Starting with v6.6., the Linux kernel supports..." comment
and rewrote the sentence afterwards as follows:

"Dependent on the target in case the shadow stack pointer is unavailable, the ssp
register can be invalid or 0x0."

I hope this is a bit better.

> > +  if (ssp == 0x0)
> > +    return {};
> > +
> > +  return ssp;
> > +}
> > +
> > +/* If shadow stack is enabled, push the address NEW_ADDR on the
> > +shadow
> 
> s/the address NEW_ADDR on/NEW_ADDR to
> 
> > +   stack and increment the shadow stack pointer accordingly.  */
> > +
> > +static void
> > +amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR
> new_addr,
> > +			       regcache *regcache)
> > +{
> > +  std::optional<CORE_ADDR> ssp
> > +    = amd64_linux_get_shadow_stack_pointer (gdbarch, regcache);
> > +  if (!ssp.has_value ())
> > +    return;
> > +
> > +  /* The shadow stack grows downwards.  To push addresses on the
> > + stack,
> 
> s/on the/to the
> 
> > +     we need to decrement SSP.    */
> > +  const int element_size
> > +    = amd64_linux_shadow_stack_element_size_aligned (gdbarch);  const
> > + CORE_ADDR new_ssp = *ssp - element_size;
> > +
> > +  /* Starting with v6.6., the Linux kernel supports CET shadow stack.
> 
> Same typo, period after "v6.6". I feel this comment has been repeated
> enough times throughout the code. If it is available at a visible location, I
> think we can do without it elsewhere.

I agree.

> For instance, we could even mention the
> kernel version in the news entry, or at the gdbarch initialization code when
> we are fetching a target description. Then it should be enough.

I would suggest to remove the comment

"Starting with v6.6., the Linux kernel supports CET shadow stack."

everywhere except in the allow_ssp_tests procedure, as it fits best there in my opinion.

Would that be acceptable?

<...>

> Otherwise this is OK.
> 
> Reviewed-By: Luis Machado <luis.machado@arm.com>

I actually missed to reply to this feedback in the first place. Sorry for the late reply.
I'll post my v5 soon, as I'll be out for one week.  If anything suggested above is not acceptable, 
it would be great if you could let me know. 
Since this is affecting comments only, I think it should be straight-forward to fix.
For now I'll post my v5 as suggested above.

Kind Regards,
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