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

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

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Thursday, January 30, 2025 11:34 AM
> To: Guinevere Larsen <guinevere@redhat.com>; Schimpe, Christina
> <christina.schimpe@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 11/12] gdb, gdbarch: Introduce gdbarch method to get the
> shadow stack pointer.
> 
> On 1/28/25 20:27, Guinevere Larsen wrote:
> > 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.
> >
> 
> As a counterpoint, I suppose it is fine to do it this way for simplicity of reviewing,
> as it is broken up into smaller bits. Things will potentially be pushed as part of a
> series anyway.

Thanks for the feedback!

In my opinion, splitting the commits simplifies the review, especially if the commits are bigger. 
Also I thought it's good to keep target independent code separate.

For the commit " gdb, gdbarch: Enable inferior calls for shadow stack support.", for instance,  
I do not have 2 commits for the gdbarch function and it's usage, as in this case the gdbarch
function is called in target independent code only. I only have the implementation in a separate
commit "gdb: Implement amd64 linux shadow stack support for inferior calls.".

But I understand Guinevere's suggestion here, especially because the commit that uses
gdbarch_get_shadow_stack_pointer is only called in a patch which isn't really too big to review.

So I have mixed feelings about that and would be happy about a guideline. 

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