[PATCH] Powerpc: Add support for openat and fstatat syscalls

[PATCH] Powerpc: Add support for openat and fstatat syscalls

[Carl Love]

GDB maintainers:

The following patch adds the sycall support for the openat and fstatat
system calls.  The missing support results in gdb.reverse/fstatat-
reverse.exp reporting a failure.

The following patch adds the support.  The testcase runs without any
failures.  The patch was tested on a Power 9 system.

Please let me know if the patch is acceptable for mainline.  Thanks.

                        Carl 


-----------------------------------------------
Powerpc: Add support for openat and fstatat syscalls

[gdb] update ppc-linux-tdep.c

Add else if syscall entries for the openat and fstatat system calls.
---
 gdb/ppc-linux-tdep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index ea8e3b98fa4..d49e142494f 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1391,6 +1391,10 @@ ppc_canonicalize_syscall (int syscall)
     result = syscall += 259 - 240;
   else if (syscall >= 250 && syscall <= 251)	/* tgkill */
     result = syscall + 270 - 250;
+  else if (syscall == 286)
+    result = gdb_sys_openat;
+  else if (syscall == 291)
+    result = gdb_sys_fstatat64;
   else if (syscall == 336)
     result = gdb_sys_recv;
   else if (syscall == 337)
-- 
2.25.1

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Ulrich Weigand]

"Carl Love" <cel@us.ibm.com> wrote on 05.10.2021 22:59:06:

> +  else if (syscall == 286)
> +    result = gdb_sys_openat;

This looks OK, but ...

> +  else if (syscall == 291)
> +    result = gdb_sys_fstatat64;

syscall 291 is actually different between 32-bit
and 64-bit: on 32-bit it is fstatat64, but on
64-bit it is newfstatat.

Given that this routine seems to be used for
both flavors, it should be correct for both.

(Also, there seem to be many more syscalls that
are not handled even though they could be.  But
that can be left for another time I guess ...)

Bye,
Ulrich

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Sergio Durigan Junior]

On Thursday, October 07 2021, Ulrich Weigand via Gdb-patches wrote:

> "Carl Love" <cel@us.ibm.com> wrote on 05.10.2021 22:59:06:
>
>> +  else if (syscall == 286)
>> +    result = gdb_sys_openat;
>
> This looks OK, but ...
>
>> +  else if (syscall == 291)
>> +    result = gdb_sys_fstatat64;
>
> syscall 291 is actually different between 32-bit
> and 64-bit: on 32-bit it is fstatat64, but on
> 64-bit it is newfstatat.
>
> Given that this routine seems to be used for
> both flavors, it should be correct for both.
>
> (Also, there seem to be many more syscalls that
> are not handled even though they could be.  But
> that can be left for another time I guess ...)

As a side note, and something that has bothered me for many years now:
this entire file relies on hardcoded syscall values, while GDB maintains
XML files for syscalls in several different architectures due to the
"catch syscall" command.  Ideally the reverse/record feature should be
rewritten to use those XML files instead.

Ref.: https://sourceware.org/bugzilla/show_bug.cgi?id=17402

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Carl Love]

Ulrich:

On Thu, 2021-10-07 at 19:52 +0200, Ulrich Weigand wrote:
> "Carl Love" <cel@us.ibm.com> wrote on 05.10.2021 22:59:06:
> 
> > +  else if (syscall == 286)
> > +    result = gdb_sys_openat;
> 
> This looks OK, but ...
> 
> > +  else if (syscall == 291)
> > +    result = gdb_sys_fstatat64;
> 
> syscall 291 is actually different between 32-bit
> and 64-bit: on 32-bit it is fstatat64, but on
> 64-bit it is newfstatat.
> 
> Given that this routine seems to be used for
> both flavors, it should be correct for both.
> 
> (Also, there seem to be many more syscalls that
> are not handled even though they could be.  But
> that can be left for another time I guess ...)

I changed the fstatat64 to newfstat.  I re-ran the regression tests on
a Power 9 system.  The patch does seem to work correctly.  Don't have a
32-bit system to verify on.

Please let me know if the patch below looks acceptable.  Thanks.

                       Carl 

-------------------------------------------------------
Powerpc: Add support for openat and fstatat syscalls

[gdb] update ppc-linux-tdep.c

Add else if syscall entries for the openat and fstatat system calls.
---
 gdb/ppc-linux-tdep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 1e94922f25a..895dacd7c1b 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1391,6 +1391,10 @@ ppc_canonicalize_syscall (int syscall)
     result = syscall += 259 - 240;
   else if (syscall >= 250 && syscall <= 251)	/* tgkill */
     result = syscall + 270 - 250;
+  else if (syscall == 286)
+    result = gdb_sys_openat;
+  else if (syscall == 291)
+    result = gdb_sys_newfstatat;
   else if (syscall == 336)
     result = gdb_sys_recv;
   else if (syscall == 337)
-- 
2.25.1

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[will schmidt]

On Mon, 2021-10-11 at 14:17 -0700, Carl Love wrote:
> Ulrich:
> 
> On Thu, 2021-10-07 at 19:52 +0200, Ulrich Weigand wrote:
> > "Carl Love" <cel@us.ibm.com> wrote on 05.10.2021 22:59:06:
> > 
> > > +  else if (syscall == 286)
> > > +    result = gdb_sys_openat;
> > 
> > This looks OK, but ...
> > 
> > > +  else if (syscall == 291)
> > > +    result = gdb_sys_fstatat64;
> > 
> > syscall 291 is actually different between 32-bit
> > and 64-bit: on 32-bit it is fstatat64, but on
> > 64-bit it is newfstatat.

Noting this comment for below.

> > 
> > Given that this routine seems to be used for
> > both flavors, it should be correct for both.
> > 
> > (Also, there seem to be many more syscalls that
> > are not handled even though they could be.  But
> > that can be left for another time I guess ...)
> 
> I changed the fstatat64 to newfstat.  I re-ran the regression tests
> on
> a Power 9 system.  The patch does seem to work correctly.  Don't have
> a
> 32-bit system to verify on.

So.. the regression tests passed with both fstatat64 and newfstat ? 
That suggests there is not a significant difference between the two
syscalls, or this particular corner is not being tested.   

> 
> Please let me know if the patch below looks acceptable.  Thanks.
> 
>                        Carl 
> 
> -------------------------------------------------------
> Powerpc: Add support for openat and fstatat syscalls
> 
> [gdb] update ppc-linux-tdep.c
> 
> Add else if syscall entries for the openat and fstatat system calls.
> ---
>  gdb/ppc-linux-tdep.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 1e94922f25a..895dacd7c1b 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -1391,6 +1391,10 @@ ppc_canonicalize_syscall (int syscall)
>      result = syscall += 259 - 240;
>    else if (syscall >= 250 && syscall <= 251)	/* tgkill */
>      result = syscall + 270 - 250;
> +  else if (syscall == 286)
> +    result = gdb_sys_openat;
> +  else if (syscall == 291)
> +    result = gdb_sys_newfstatat;


The above comment suggests that there needs to be some sort of if/else
logic here to handle 32- or 64- bit.  

If there is rationale for fixing for one, and leaving broken or
incorrect for the other, some commentary should be added.


Thanks
-Will



>    else if (syscall == 336)
>      result = gdb_sys_recv;
>    else if (syscall == 337)

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Carl Love]

Will:


> > I changed the fstatat64 to newfstat.  I re-ran the regression tests
> > on
> > a Power 9 system.  The patch does seem to work correctly.  Don't
> > have
> > a
> > 32-bit system to verify on.
> 
> So.. the regression tests passed with both fstatat64 and newfstat ? 
> That suggests there is not a significant difference between the two
> syscalls, or this particular corner is not being tested.   
> 

It seemed weird to me that both work.  I too was a bit puzzled and
concerned by that.  So I did some more digging .... 

I found the following web page that gives the syscall numbers on a
number of platforms

https://marcin.juszkiewicz.com.pl/download/tables/syscalls.html

So as Ulrich said fstatat64 is listed for powerpc (32-bit) but not
supported for powerpc64.  The fstatat64 call maps to system call number
291.

Looking at newfstatat in the table, it is defined on powerpc64 but not
on powerpc (32-bit).  It also maps to system call number 291.

Clicking on newfstatat and fstatat64 in the left column takes us to the
same man page for both system call names.  

The man pages says is:  

"The fstatat() system call is a more general interface for accessing
file information which can still provide exactly the behavior of each
of stat(), lstat(), and fstat()."  

So, it says to me that calling newfstatat for both, as Ulrich said, is
fine.  I do think this is all rather convoluted.  I guess it is one of
those things that evolved over time into something more convoulted and
confusing then it should be.  

So as you said "...there is not a significant difference between the
two syscalls".  

                        Carl 

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Ulrich Weigand]

"Carl Love" <cel@us.ibm.com> wrote on 12.10.2021 21:13:43:

> So, it says to me that calling newfstatat for both, as Ulrich said, is
> fine.  I do think this is all rather convoluted.  I guess it is one of
> those things that evolved over time into something more convoulted and
> confusing then it should be.
>
> So as you said "...there is not a significant difference between the
> two syscalls".

The two syscalls are similar, but not identical.  In particular, they
differ in the amount of memory that is accessed:

 sys_newfstatat accesses a "struct stat"
    (144 bytes on 64-bit / 88 bytes on 32-bit)
 sys_fstatat64 accesses a "struct stat64"
    (104 bytes on both 64-bit and 32-bit)

Since this affects the memory being recorded, it would actually be
preferable to use the correct system call depending on ABI.

Note that in general, there are more cases of syscall numbers refering
to different system calls depending on the ABI (64-bit vs. 32-bit),
so it would make sense to add support for that in general.

As an example, you may want to look s390_canonicalize_syscall,
which does make the distinction between 64-bit and 32-bit ABI.


Bye,
Ulrich

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Carl Love]

Ulrich:

On Wed, 2021-10-13 at 15:08 +0200, Ulrich Weigand wrot
> 
> The two syscalls are similar, but not identical.  In particular, they
> differ in the amount of memory that is accessed:
> 
>  sys_newfstatat accesses a "struct stat"
>     (144 bytes on 64-bit / 88 bytes on 32-bit)
>  sys_fstatat64 accesses a "struct stat64"
>     (104 bytes on both 64-bit and 32-bit)
> 
> Since this affects the memory being recorded, it would actually be
> preferable to use the correct system call depending on ABI.
> 
> Note that in general, there are more cases of syscall numbers
> refering
> to different system calls depending on the ABI (64-bit vs. 32-bit),
> so it would make sense to add support for that in general.
> 
> As an example, you may want to look s390_canonicalize_syscall,
> which does make the distinction between 64-bit and 32-bit ABI

Thanks for the additional info and pointers.  I added a word size
argument to ppc_canonicalize_syscall.  The argument is then used to
select the correct 64-bit or 32-bit syscall.  

The updated patch has been retested on Power 10.  Please let me know if
it looks OK now.  Thanks.

                    Carl 


 --------------------------------------------------------
Powerpc: Add support for openat and fstatat syscalls

[gdb] update ppc-linux-tdep.c

Add argument to ppc_canonicalize_syscall for the wordsize.
Add syscall entries for the openat and fstatat system calls.
---
 gdb/ppc-linux-tdep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index ea8e3b98fa4..1e0e9a0268a 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1371,7 +1371,7 @@ static struct linux_record_tdep ppc64_linux_record_tdep;
    SYSCALL.  */
 
 static enum gdb_syscall
-ppc_canonicalize_syscall (int syscall)
+ppc_canonicalize_syscall (int syscall, int wordsize)
 {
   int result = -1;
 
@@ -1391,6 +1391,13 @@ ppc_canonicalize_syscall (int syscall)
     result = syscall += 259 - 240;
   else if (syscall >= 250 && syscall <= 251)	/* tgkill */
     result = syscall + 270 - 250;
+  else if (syscall == 286)
+    result = gdb_sys_openat;
+  else if (syscall == 291)
+    if (wordsize == 64)
+      result = gdb_sys_newfstatat;
+    else
+      result = gdb_sys_fstatat64;
   else if (syscall == 336)
     result = gdb_sys_recv;
   else if (syscall == 337)
@@ -1414,7 +1421,7 @@ ppc_linux_syscall_record (struct regcache *regcache)
   int ret;
 
   regcache_raw_read_unsigned (regcache, tdep->ppc_gp0_regnum, &scnum);
-  syscall_gdb = ppc_canonicalize_syscall (scnum);
+  syscall_gdb = ppc_canonicalize_syscall (scnum, tdep->wordsize);
 
   if (syscall_gdb < 0)
     {
-- 
2.30.2

Re: [PATCH] Powerpc: Add support for openat and fstatat syscalls

[Ulrich Weigand]

"Carl Love" <cel@us.ibm.com> wrote on 13.10.2021 23:55:43:

> Powerpc: Add support for openat and fstatat syscalls
>
> [gdb] update ppc-linux-tdep.c
>
> Add argument to ppc_canonicalize_syscall for the wordsize.
> Add syscall entries for the openat and fstatat system calls.

> +  else if (syscall == 286)
> +    result = gdb_sys_openat;
> +  else if (syscall == 291)
> +    if (wordsize == 64)
> +      result = gdb_sys_newfstatat;
> +    else
> +      result = gdb_sys_fstatat64;

Please add braces around the nested if to avoid the
ambiguous else.

Otherwise, this is OK.

Thanks,
Ulrich