[gdb/tdep] Backport i386_canonicalize_syscall rewrite to gdb-16-branch

  Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") fixes
PR32770, which reproduces on the gdb-16-branch, but the commit is not ideal
for backporting because it completely rewrites i386_canonicalize_syscall.

Instead, this is a version of the patch that adds a single line entry for each
syscall value for which i386_canonicalize_syscall gives a different result
with and without the patch.

Consequently, the two versions give identical results.  I've checked this for
syscalls 0 to 466.

Tested on x86_64-linux with target board unix/-m32, on top of gdb-16-branch.

PR tdep/32770
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
---
 gdb/i386-linux-tdep.c | 153 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 1 deletion(-)


base-commit: 465465ce9ca4f3e6e29aeaf33f6a2220693f4e62
  

On 3/13/25 6:53 AM, Tom de Vries wrote:
> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") fixes
> PR32770, which reproduces on the gdb-16-branch, but the commit is not ideal
> for backporting because it completely rewrites i386_canonicalize_syscall.
>
> Instead, this is a version of the patch that adds a single line entry for each
> syscall value for which i386_canonicalize_syscall gives a different result
> with and without the patch.
>
> Consequently, the two versions give identical results.  I've checked this for
> syscalls 0 to 466.
>
> Tested on x86_64-linux with target board unix/-m32, on top of gdb-16-branch.
>
> PR tdep/32770
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
> ---

Hi!

I looked over all the special cases, and it mostly looks ok. There are 
quite a few syscalls where we have an older or similar enough version, 
like execveat and execve, or faccessat and faccessat2, but I can see 
from the accept4 patch why you wouldn't have done that... however, for 
the semtimedop_time64 syscall, you just converted to the semtimedop 
version. What is special about that one?
  

On 3/13/25 13:40, Guinevere Larsen wrote:
> On 3/13/25 6:53 AM, Tom de Vries wrote:
>> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") fixes
>> PR32770, which reproduces on the gdb-16-branch, but the commit is not 
>> ideal
>> for backporting because it completely rewrites i386_canonicalize_syscall.
>>
>> Instead, this is a version of the patch that adds a single line entry 
>> for each
>> syscall value for which i386_canonicalize_syscall gives a different 
>> result
>> with and without the patch.
>>
>> Consequently, the two versions give identical results.  I've checked 
>> this for
>> syscalls 0 to 466.
>>
>> Tested on x86_64-linux with target board unix/-m32, on top of gdb-16- 
>> branch.
>>
>> PR tdep/32770
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
>> ---
> 
> Hi!
> 
> I looked over all the special cases, and it mostly looks ok. There are 
> quite a few syscalls where we have an older or similar enough version, 
> like execveat and execve, or faccessat and faccessat2, but I can see 
> from the accept4 patch why you wouldn't have done that... however, for 
> the semtimedop_time64 syscall, you just converted to the semtimedop 
> version. What is special about that one?
> 

Good question.

This was one of the syscalls I came across while reviewing the 
gdb_syscall enum values >= 500.  And since the recording support doesn't 
actually record the timeout parameter, the time64 part of 
semtimedop_time64 seemed irrelevant.

So, what was special here was that gdb_sys_semtimedop was supported, but 
there was no corresponding mapping from i386.

For the execveat case, gdb_sys_execveat is missing.  That makes it 
similar to the accept4 case.

I hope that answers your question.

Thanks,
- Tom
  

Tom de Vries <tdevries@suse.de> writes:

> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") fixes
> PR32770, which reproduces on the gdb-16-branch, but the commit is not ideal
> for backporting because it completely rewrites i386_canonicalize_syscall.
>
> Instead, this is a version of the patch that adds a single line entry for each
> syscall value for which i386_canonicalize_syscall gives a different result
> with and without the patch.
>
> Consequently, the two versions give identical results.  I've checked this for
> syscalls 0 to 466.

Given you've shown that this is equivalent to the version in
fbfb29b304e, and having taken a look at that commit, I'd be just as
happy to see that commit merged into the gdb-16-branch.  The patch isn't
small, but equally, it's pretty contained and straight forward, so I
don't see huge risk there.

But if you're happier with this approach in the gdb-16-branch, that's
fine with me.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Tested on x86_64-linux with target board unix/-m32, on top of gdb-16-branch.
>
> PR tdep/32770
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
  

On 3/13/25 10:06 AM, Tom de Vries wrote:
> On 3/13/25 13:40, Guinevere Larsen wrote:
>> On 3/13/25 6:53 AM, Tom de Vries wrote:
>>> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") 
>>> fixes
>>> PR32770, which reproduces on the gdb-16-branch, but the commit is 
>>> not ideal
>>> for backporting because it completely rewrites 
>>> i386_canonicalize_syscall.
>>>
>>> Instead, this is a version of the patch that adds a single line 
>>> entry for each
>>> syscall value for which i386_canonicalize_syscall gives a different 
>>> result
>>> with and without the patch.
>>>
>>> Consequently, the two versions give identical results.  I've checked 
>>> this for
>>> syscalls 0 to 466.
>>>
>>> Tested on x86_64-linux with target board unix/-m32, on top of 
>>> gdb-16- branch.
>>>
>>> PR tdep/32770
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
>>> ---
>>
>> Hi!
>>
>> I looked over all the special cases, and it mostly looks ok. There 
>> are quite a few syscalls where we have an older or similar enough 
>> version, like execveat and execve, or faccessat and faccessat2, but I 
>> can see from the accept4 patch why you wouldn't have done that... 
>> however, for the semtimedop_time64 syscall, you just converted to the 
>> semtimedop version. What is special about that one?
>>
>
> Good question.
>
> This was one of the syscalls I came across while reviewing the 
> gdb_syscall enum values >= 500.  And since the recording support 
> doesn't actually record the timeout parameter, the time64 part of 
> semtimedop_time64 seemed irrelevant.
>
> So, what was special here was that gdb_sys_semtimedop was supported, 
> but there was no corresponding mapping from i386.
>
> For the execveat case, gdb_sys_execveat is missing.  That makes it 
> similar to the accept4 case.

I see... so other syscalls ending with _time64 where GDB wouldn't record 
the time64 portion could just be swapped for the regular version?

I'm asking because I noticed a couple, but I didn't double check what 
exactly they recorded, but it could be a quick improvement in that case.

The reason I ask for execveat, is that we don't actually record 
anything, so for GDB it would be the same as execve, which we do 
support. I'm basically trying to narrow down when to include a gdb_sys 
version, and when I can return a close-enough syscall...
  

On 3/13/25 16:20, Guinevere Larsen wrote:
> On 3/13/25 10:06 AM, Tom de Vries wrote:
>> On 3/13/25 13:40, Guinevere Larsen wrote:
>>> On 3/13/25 6:53 AM, Tom de Vries wrote:
>>>> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") 
>>>> fixes
>>>> PR32770, which reproduces on the gdb-16-branch, but the commit is 
>>>> not ideal
>>>> for backporting because it completely rewrites 
>>>> i386_canonicalize_syscall.
>>>>
>>>> Instead, this is a version of the patch that adds a single line 
>>>> entry for each
>>>> syscall value for which i386_canonicalize_syscall gives a different 
>>>> result
>>>> with and without the patch.
>>>>
>>>> Consequently, the two versions give identical results.  I've checked 
>>>> this for
>>>> syscalls 0 to 466.
>>>>
>>>> Tested on x86_64-linux with target board unix/-m32, on top of 
>>>> gdb-16- branch.
>>>>
>>>> PR tdep/32770
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
>>>> ---
>>>
>>> Hi!
>>>
>>> I looked over all the special cases, and it mostly looks ok. There 
>>> are quite a few syscalls where we have an older or similar enough 
>>> version, like execveat and execve, or faccessat and faccessat2, but I 
>>> can see from the accept4 patch why you wouldn't have done that... 
>>> however, for the semtimedop_time64 syscall, you just converted to the 
>>> semtimedop version. What is special about that one?
>>>
>>
>> Good question.
>>
>> This was one of the syscalls I came across while reviewing the 
>> gdb_syscall enum values >= 500.  And since the recording support 
>> doesn't actually record the timeout parameter, the time64 part of 
>> semtimedop_time64 seemed irrelevant.
>>
>> So, what was special here was that gdb_sys_semtimedop was supported, 
>> but there was no corresponding mapping from i386.
>>
>> For the execveat case, gdb_sys_execveat is missing.  That makes it 
>> similar to the accept4 case.
> 
> I see... so other syscalls ending with _time64 where GDB wouldn't record 
> the time64 portion could just be swapped for the regular version?
> 
> I'm asking because I noticed a couple, but I didn't double check what 
> exactly they recorded, but it could be a quick improvement in that case.
> 
> The reason I ask for execveat, is that we don't actually record 
> anything, so for GDB it would be the same as execve, which we do 
> support. I'm basically trying to narrow down when to include a gdb_sys 
> version, and when I can return a close-enough syscall...

I think the goal is to maximize the same-name mappings, so, support 
execveat like this:
...
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 3fb1c1765e4..dcb973fd4c7 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -758,7 +758,7 @@ i386_canonicalize_syscall (int syscall)
        SYSCALL_MAP (getrandom);
        UNSUPPORTED_SYSCALL_MAP (memfd_create);
        UNSUPPORTED_SYSCALL_MAP (bpf);
-      UNSUPPORTED_SYSCALL_MAP (execveat);
+      SYSCALL_MAP (execveat);
        SYSCALL_MAP (socket);
        SYSCALL_MAP (socketpair);
        SYSCALL_MAP (bind);
diff --git a/gdb/linux-record.c b/gdb/linux-record.c
index 0b2709b30b2..c442d13a9bc 100644
--- a/gdb/linux-record.c
+++ b/gdb/linux-record.c
@@ -289,6 +289,7 @@ record_linux_system_call (enum gdb_syscall syscall,
      case gdb_sys_link:
      case gdb_sys_unlink:
      case gdb_sys_execve:
+    case gdb_sys_execveat:
      case gdb_sys_chdir:
        break;

diff --git a/gdb/linux-record.h b/gdb/linux-record.h
index 6d6ba073089..e8b33f81013 100644
--- a/gdb/linux-record.h
+++ b/gdb/linux-record.h
@@ -542,6 +542,7 @@ enum gdb_syscall {
    gdb_sys_msgctl = 531,
    gdb_sys_semtimedop = 532,
    gdb_sys_accept4 = 533,
+  gdb_sys_execveat = 534,
    gdb_sys_newfstatat = 540,
  };

diff --git a/gdb/loongarch-linux-tdep.c b/gdb/loongarch-linux-tdep.c
index 2168ce642b9..b9ba265f4f8 100644
--- a/gdb/loongarch-linux-tdep.c
+++ b/gdb/loongarch-linux-tdep.c
@@ -851,7 +851,7 @@ loongarch_canonicalize_syscall (enum 
loongarch_syscall syscall_number)
        SYSCALL_MAP (getrandom);
        UNSUPPORTED_SYSCALL_MAP (memfd_create);
        UNSUPPORTED_SYSCALL_MAP (bpf);
-      UNSUPPORTED_SYSCALL_MAP (execveat);
+      SYSCALL_MAP (execveat);
        UNSUPPORTED_SYSCALL_MAP (userfaultfd);
        UNSUPPORTED_SYSCALL_MAP (membarrier);
        UNSUPPORTED_SYSCALL_MAP (mlock2);
...

Thanks,
- Tom
  

On 3/13/25 16:09, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") fixes
>> PR32770, which reproduces on the gdb-16-branch, but the commit is not ideal
>> for backporting because it completely rewrites i386_canonicalize_syscall.
>>
>> Instead, this is a version of the patch that adds a single line entry for each
>> syscall value for which i386_canonicalize_syscall gives a different result
>> with and without the patch.
>>
>> Consequently, the two versions give identical results.  I've checked this for
>> syscalls 0 to 466.
> 
> Given you've shown that this is equivalent to the version in
> fbfb29b304e, and having taken a look at that commit, I'd be just as
> happy to see that commit merged into the gdb-16-branch.  The patch isn't
> small, but equally, it's pretty contained and straight forward, so I
> don't see huge risk there.
> 
> But if you're happier with this approach in the gdb-16-branch, that's
> fine with me.
> 

Hi Andrew,

thanks for the review.

Gwen, do you have a preference?

If not, given Andrew's assessment, I'll pick the patch from master 
rather than this one.

Thanks,
- Tom

> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew
> 
> 
>>
>> Tested on x86_64-linux with target board unix/-m32, on top of gdb-16-branch.
>>
>> PR tdep/32770
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32770
>
  

On 3/14/25 12:46 PM, Tom de Vries wrote:
> On 3/13/25 16:09, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> Commit fbfb29b304e ("[gdb/tdep] Rewrite i386_canonicalize_syscall") 
>>> fixes
>>> PR32770, which reproduces on the gdb-16-branch, but the commit is 
>>> not ideal
>>> for backporting because it completely rewrites 
>>> i386_canonicalize_syscall.
>>>
>>> Instead, this is a version of the patch that adds a single line 
>>> entry for each
>>> syscall value for which i386_canonicalize_syscall gives a different 
>>> result
>>> with and without the patch.
>>>
>>> Consequently, the two versions give identical results.  I've checked 
>>> this for
>>> syscalls 0 to 466.
>>
>> Given you've shown that this is equivalent to the version in
>> fbfb29b304e, and having taken a look at that commit, I'd be just as
>> happy to see that commit merged into the gdb-16-branch.  The patch isn't
>> small, but equally, it's pretty contained and straight forward, so I
>> don't see huge risk there.
>>
>> But if you're happier with this approach in the gdb-16-branch, that's
>> fine with me.
>>
>
> Hi Andrew,
>
> thanks for the review.
>
> Gwen, do you have a preference?
>
> If not, given Andrew's assessment, I'll pick the patch from master 
> rather than this one.
Not really, feel free to go with Andrew's suggestion :)