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

[Guinevere Larsen <guinevere@redhat.com>]

On 1/30/25 9:34 AM, Schimpe, Christina wrote:
>> -----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.

Yeah having had the time to read patch 12, I do think they'd go best 
together, but I don't know if there is any unified guidance on this.

Whatever you decide is fine, though, I don't have too strong an opinion 
on this.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers