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

[Tom de Vries <tdevries@suse.de>]

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