Re: [PATCH] Fix amd64->i386 linux syscall restart problem

[Pedro Alves <palves at redhat dot com>]

On 4/10/19 3:29 AM, Kevin Buettner wrote:
> On Tue, 9 Apr 2019 18:46:45 +0100
> Pedro Alves <palves@redhat.com> wrote:
> 
>>> +	  void *ptr = (gdb_byte *) gregs 
>>> +		    + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM];  
>>
>> Need to wrap the expression that spawns two lines in ()s.
> 
> I've made this change.  And the other one too.
> 
>>> +
>>> +  /* Sign extend EAX value to avoid potential syscall restart problems.  */  
>>
>> I'd rather see both implementations use the same comment.  Could you
>> merge them?
> 
> I started to merge them and then decided to write a more detailed
> comment based on the text that I wrote for the commit comment.  I
> have, for the moment anyway, copied the comment to both locations with
> only slight changes which reflect where the comment is located.  The
> problem with having copies of the same long comment in two or more
> places is making sure that if one gets updated, then the rest do too. 
> It might be better to have one refer to the other.
> 
> I'm thinking that it might be preferable to have something like this
> in gdb/gdbserver/linux-x86-low.c:
> 
>   /* Sign extend EAX value to avoid potential syscall restart problems.
>   
>      See amd64_linux_collect_native_gregset() in gdb/amd64-linux-nat.c
>      for a detailed explanation.  */

I'm OK with that.  My concern with different comments in two places
is that at some point, like we managed to merge the x86
watchpoints code from gdb and gdbserver under gdb/nat/, we might be
able to merge this code as well.  And different comments make the job
a bit harder for the person doing that, since the person _then_ has to
decide what the resulting comment will be like.  If we can do it now,
it's preferable.  A pointer from one end to the other, like you're
proposing, works for me too.

> Below is a diff showing the new comments.  It also includes the
> changes which wrap the multi-line expressions in parens.

Thanks, that new version of the comment looks great.

Pedro Alves