Mips: Fix kernel_stat structure size

Mips: Fix kernel_stat structure size

[Dimitrije Milosevic]

Fix kernel_stat structure size for non-Android 32-bit Mips.
LLVM currently has this value for the kernel_stat structure size,
as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h.
This also resolves one of the build issues for non-Android 32-bit Mips.

    libsanitizer/ChangeLog:
    
            * sanitizer_common/sanitizer_platform_limits_posix.h: Fix
            kernel_stat structure size for non-Android 32-bit Mips.

---

 libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
index 89772a7e5c0..62a99035db3 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__mips__)
 const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID
                                            ? FIRST_32_SECOND_64(104, 128)
-                                           : FIRST_32_SECOND_64(144, 216);
+                                           : FIRST_32_SECOND_64(160, 216);
 const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__s390__) && !defined(__s390x__)
 const unsigned struct_kernel_stat_sz = 64;

---

Re: Mips: Fix kernel_stat structure size

[Xi Ruoyao]

On Fri, 2022-07-01 at 12:40 +0000, Dimitrije Milosevic wrote:
> Fix kernel_stat structure size for non-Android 32-bit Mips.
> LLVM currently has this value for the kernel_stat structure size,
> as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h.
> This also resolves one of the build issues for non-Android 32-bit Mips.

nit: the ChangeLog file name shall have no indents in the commit
message, and there should be one tab (instead of 8 whitespaces) before
the content.  Like:

libsanitizer/ChangeLog:

	* sanitizer_common/sanitizer_platform_limits_posix.h: Fix
	kernel_stat structure size for non-Android 32-bit Mips.

Patch content LGTM as it just changes our code to match the upstream,
but I don't have privilege to approve the change.  Richard?

>     libsanitizer/ChangeLog:
>     
>             * sanitizer_common/sanitizer_platform_limits_posix.h: Fix
>             kernel_stat structure size for non-Android 32-bit Mips.
> 
> ---
> 
>  libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
> index 89772a7e5c0..62a99035db3 100644
> --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
> +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
> @@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104;
>  #elif defined(__mips__)
>  const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID
>                                             ? FIRST_32_SECOND_64(104, 128)
> -                                           : FIRST_32_SECOND_64(144, 216);
> +                                           : FIRST_32_SECOND_64(160, 216);
>  const unsigned struct_kernel_stat64_sz = 104;
>  #elif defined(__s390__) && !defined(__s390x__)
>  const unsigned struct_kernel_stat_sz = 64;
> 
> ---

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: Mips: Fix kernel_stat structure size

[Dimitrije Milosevic]

Thanks Xi. Forgive me as I'm not that familiar with the coding standards 
when submitting patches for a review.
Here is the updated version of the patch.

Fix kernel_stat structure size for non-Android 32-bit Mips.
LLVM currently has this value for the kernel_stat structure size,
as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h.
This also resolves one of the build issues for non-Android 32-bit Mips.

libsanitizer/ChangeLog:

	* sanitizer_common/sanitizer_platform_limits_posix.h: Fix
	kernel_stat structure size for non-Android 32-bit Mips.

---

 libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
index 89772a7e5c0..62a99035db3 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__mips__)
 const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID
                                            ? FIRST_32_SECOND_64(104, 128)
-                                           : FIRST_32_SECOND_64(144, 216);
+                                           : FIRST_32_SECOND_64(160, 216);
 const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__s390__) && !defined(__s390x__)
 const unsigned struct_kernel_stat_sz = 64;

---

Re: Mips: Fix kernel_stat structure size

[Dimitrije Milosevic]

Ping. :)
I think this is good to go. Unfortunately, I do not have commit access, so if anyone
can commit it, that would be great!


From: Dimitrije Milosevic <Dimitrije.Milosevic@Syrmia.com>
Sent: Friday, July 1, 2022 4:25 PM
To: Xi Ruoyao <xry111@xry111.site>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Cc: Djordje Todorovic <Djordje.Todorovic@syrmia.com>; Richard Sandiford <richard.sandiford@arm.com>
Subject: Re: Mips: Fix kernel_stat structure size 
 
Thanks Xi. Forgive me as I'm not that familiar with the coding standards 
when submitting patches for a review.
Here is the updated version of the patch.

Fix kernel_stat structure size for non-Android 32-bit Mips.
LLVM currently has this value for the kernel_stat structure size,
as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h.
This also resolves one of the build issues for non-Android 32-bit Mips.

libsanitizer/ChangeLog:

        * sanitizer_common/sanitizer_platform_limits_posix.h: Fix
        kernel_stat structure size for non-Android 32-bit Mips.

---

 libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
index 89772a7e5c0..62a99035db3 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__mips__)
 const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID
                                            ? FIRST_32_SECOND_64(104, 128)
-                                           : FIRST_32_SECOND_64(144, 216);
+                                           : FIRST_32_SECOND_64(160, 216);
 const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__s390__) && !defined(__s390x__)
 const unsigned struct_kernel_stat_sz = 64;

---

Re: Mips: Fix kernel_stat structure size

[Hans-Peter Nilsson]

On Fri, 1 Jul 2022, Dimitrije Milosevic wrote:

> Fix kernel_stat structure size for non-Android 32-bit Mips.
> LLVM currently has this value for the kernel_stat structure size,
> as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h.
> This also resolves one of the build issues for non-Android 32-bit Mips.

I insist that PR105614 comment #7 is the way to go, i.e. fix
the merge error, avoiding the faulty include that it
reintroduced.  Was this tested on O32?

>
>     libsanitizer/ChangeLog:
>
>             * sanitizer_common/sanitizer_platform_limits_posix.h: Fix
>             kernel_stat structure size for non-Android 32-bit Mips.
>
> ---
>
>  libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
> index 89772a7e5c0..62a99035db3 100644
> --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
> +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
> @@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104;
>  #elif defined(__mips__)
>  const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID
>                                             ? FIRST_32_SECOND_64(104, 128)
> -                                           : FIRST_32_SECOND_64(144, 216);
> +                                           : FIRST_32_SECOND_64(160, 216);
>  const unsigned struct_kernel_stat64_sz = 104;
>  #elif defined(__s390__) && !defined(__s390x__)
>  const unsigned struct_kernel_stat_sz = 64;
>
> ---

Re: Mips: Fix kernel_stat structure size

[Xi Ruoyao]

On Fri, 2022-07-08 at 21:42 -0400, Hans-Peter Nilsson wrote:
> On Fri, 1 Jul 2022, Dimitrije Milosevic wrote:
> 
> > Fix kernel_stat structure size for non-Android 32-bit Mips.
> > LLVM currently has this value for the kernel_stat structure size,
> > as per compiler-rt/lib/sanitizer-
> > common/sanitizer_platform_limits_posix.h.
> > This also resolves one of the build issues for non-Android 32-bit
> > Mips.
> 
> I insist that PR105614 comment #7 is the way to go, i.e. fix
> the merge error, avoiding the faulty include that it
> reintroduced.  Was this tested on O32?

I'm pretty sure it is *not* the way to go.

Sanitizer does not really intercept system call.  It intercepts libc
stat() or lstat() etc. calls.  So you need to keep struct_kernel_stat_sz
same as the size of struct stat in libc, i. e. "the size of buffer which
*libc* stat()-like functions writing into".

The "kernel_" in the name is just misleading.

And, if you still think it should be the way to go, let's submit the
change to LLVM and get it reviewed properly.
-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: Mips: Fix kernel_stat structure size

[Hans-Peter Nilsson]

On Sat, 9 Jul 2022, Xi Ruoyao wrote:

> On Fri, 2022-07-08 at 21:42 -0400, Hans-Peter Nilsson wrote:
> > On Fri, 1 Jul 2022, Dimitrije Milosevic wrote:
> >
> > > Fix kernel_stat structure size for non-Android 32-bit Mips.
> > > LLVM currently has this value for the kernel_stat structure size,
> > > as per compiler-rt/lib/sanitizer-
> > > common/sanitizer_platform_limits_posix.h.
> > > This also resolves one of the build issues for non-Android 32-bit
> > > Mips.
> >
> > I insist that PR105614 comment #7 is the way to go, i.e. fix
> > the merge error, avoiding the faulty include that it
> > reintroduced.? Was this tested on O32?
>
> I'm pretty sure it is *not* the way to go.
>
> Sanitizer does not really intercept system call.  It intercepts libc
> stat() or lstat() etc. calls.  So you need to keep struct_kernel_stat_sz
> same as the size of struct stat in libc, i. e. "the size of buffer which
> *libc* stat()-like functions writing into".
>
> The "kernel_" in the name is just misleading.

You make a sound argument, and I just refer to my old commit
9f943b2446f2d0a.  Either way, the proof is in the pussing: was
this tested for O32 or not?

> And, if you still think it should be the way to go, let's submit the
> change to LLVM and get it reviewed properly.

Sorry, I've no longer interest in mips besides I'd like to see
un-broken what I helped getting in a working state (support ASAN
in gcc for mips O32).

brgds, H-P

Re: Mips: Fix kernel_stat structure size

[Dimitrije Milosevic]

Hi Hans-Peter,
You're right, this is not ok for the O32 ABI. Your change however, broke the functionality
for the N32 ABI. AFAIK, the changes like this should go through LLVM first (yours didn't),
so I will send out a review, covering both 32 ABIs, meaning that both O32 and N32 ABIs 
will be working properly. As for this change, I'm not sure what should be done? 
Should this be committed now, while the LLVM change is cherry-picked once it's committed.
Best regards,
Dimitrije Milosevic


From: Hans-Peter Nilsson <hp@bitrange.com>
Sent: Saturday, July 9, 2022 4:44 PM
To: Xi Ruoyao <xry111@xry111.site>
Cc: Dimitrije Milosevic <Dimitrije.Milosevic@Syrmia.com>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Subject: Re: Mips: Fix kernel_stat structure size 
 
On Sat, 9 Jul 2022, Xi Ruoyao wrote:

> On Fri, 2022-07-08 at 21:42 -0400, Hans-Peter Nilsson wrote:
> > On Fri, 1 Jul 2022, Dimitrije Milosevic wrote:
> >
> > > Fix kernel_stat structure size for non-Android 32-bit Mips.
> > > LLVM currently has this value for the kernel_stat structure size,
> > > as per compiler-rt/lib/sanitizer-
> > > common/sanitizer_platform_limits_posix.h.
> > > This also resolves one of the build issues for non-Android 32-bit
> > > Mips.
> >
> > I insist that PR105614 comment #7 is the way to go, i.e. fix
> > the merge error, avoiding the faulty include that it
> > reintroduced.? Was this tested on O32?
>
> I'm pretty sure it is *not* the way to go.
>
> Sanitizer does not really intercept system call.  It intercepts libc
> stat() or lstat() etc. calls.  So you need to keep struct_kernel_stat_sz
> same as the size of struct stat in libc, i. e. "the size of buffer which
> *libc* stat()-like functions writing into".
>
> The "kernel_" in the name is just misleading.

You make a sound argument, and I just refer to my old commit
9f943b2446f2d0a.  Either way, the proof is in the pussing: was
this tested for O32 or not?

> And, if you still think it should be the way to go, let's submit the
> change to LLVM and get it reviewed properly.

Sorry, I've no longer interest in mips besides I'd like to see
un-broken what I helped getting in a working state (support ASAN
in gcc for mips O32).

brgds, H-P

Re: Mips: Fix kernel_stat structure size

[Hans-Peter Nilsson]

On Tue, 12 Jul 2022, Dimitrije Milosevic wrote:
> Hi Hans-Peter,
> You're right, this is not ok for the O32 ABI. Your change however, broke the functionality
> for the N32 ABI.

That's just not true.  There was no mips support for ASAN in
gcc, hence nothing to break for N32.  Maybe an anachronism?

Or maybe you mean plain backporting to LLVM?  Perhaps it needs
adjustments for their _FILE_OFFSET_BITS=64 builds?  On the other
hand, that struct_kernel_stat_sz shouldn't be affected by that -
unless that's a misnomer or they get the includes wrong - but
then again, the include part was fixed by the patch.  Unless
mis-merged, as in gcc currently.

> AFAIK, the changes like this should go through LLVM first
> (yours didn't),

As I recall, and judging from the mentioned commit, this would
have given them no benefit; no visible error to fix, as they're
always building the runtime with for 64-bit file size.  But yes,
it should have been submitted there eventually.

> so I will send out a review, covering both 32 ABIs, meaning that both O32 and N32 ABIs
> will be working properly.

Very good.  Thank you in advance.

> As for this change, I'm not sure what should be done?
> Should this be committed now, while the LLVM change is cherry-picked once it's committed.
> Best regards,
> Dimitrije Milosevic
>
>
> From: Hans-Peter Nilsson <hp@bitrange.com>
> Sent: Saturday, July 9, 2022 4:44 PM
> To: Xi Ruoyao <xry111@xry111.site>
> Cc: Dimitrije Milosevic <Dimitrije.Milosevic@Syrmia.com>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
> Subject: Re: Mips: Fix kernel_stat structure size
> ?
> On Sat, 9 Jul 2022, Xi Ruoyao wrote:
>
> > On Fri, 2022-07-08 at 21:42 -0400, Hans-Peter Nilsson wrote:
> > > On Fri, 1 Jul 2022, Dimitrije Milosevic wrote:
> > >
> > > > Fix kernel_stat structure size for non-Android 32-bit Mips.
> > > > LLVM currently has this value for the kernel_stat structure size,
> > > > as per compiler-rt/lib/sanitizer-
> > > > common/sanitizer_platform_limits_posix.h.
> > > > This also resolves one of the build issues for non-Android 32-bit
> > > > Mips.
> > >
> > > I insist that PR105614 comment #7 is the way to go, i.e. fix
> > > the merge error, avoiding the faulty include that it
> > > reintroduced.? Was this tested on O32?
> >
> > I'm pretty sure it is *not* the way to go.
> >
> > Sanitizer does not really intercept system call.? It intercepts libc
> > stat() or lstat() etc. calls.? So you need to keep struct_kernel_stat_sz
> > same as the size of struct stat in libc, i. e. "the size of buffer which
> > *libc* stat()-like functions writing into".
> >
> > The "kernel_" in the name is just misleading.
>
> You make a sound argument, and I just refer to my old commit
> 9f943b2446f2d0a.? Either way, the proof is in the pussing: was
> this tested for O32 or not?
>
> > And, if you still think it should be the way to go, let's submit the
> > change to LLVM and get it reviewed properly.
>
> Sorry, I've no longer interest in mips besides I'd like to see
> un-broken what I helped getting in a working state (support ASAN
> in gcc for mips O32).
>
> brgds, H-P

Re: Mips: Fix kernel_stat structure size

[Xi Ruoyao]

On Tue, 2022-07-12 at 06:42 +0000, Dimitrije Milosevic wrote:
> Hi Hans-Peter,
> You're right, this is not ok for the O32 ABI. Your change however, broke the functionality
> for the N32 ABI. AFAIK, the changes like this should go through LLVM first (yours didn't),
> so I will send out a review, covering both 32 ABIs, meaning that both O32 and N32 ABIs 
> will be working properly. As for this change, I'm not sure what should be done? 
> Should this be committed now, while the LLVM change is cherry-picked once it's committed.

I think just get it into LLVM first, then we sync with LLVM.

It seems on o32 sizeof(struct stat) is 144, n32 -> 160, n64 -> 216
(tested on gcc23.fsffrance.org).  "Luckily" we don't support other
strange things like "o64".

Re: Mips: Fix kernel_stat structure size

[Xi Ruoyao]

On Tue, 2022-07-12 at 06:42 +0000, Dimitrije Milosevic wrote:

> I will send out a review, covering both 32 ABIs, meaning that both O32
> and N32 ABIs will be working properly.

Please CC me (@xry111) in LLVM review.

By the way, if you have some spare time, you can also add the value for
Musl (all 3 ABIs).  Musl has a different struct stat than Glibc (it's PR
106136 in GCC bugzilla).

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: Mips: Fix kernel_stat structure size

[Dimitrije Milosevic]

Hi Xi,
> Please CC me (@xry111) in LLVM review.
I just posted the patch. You should've gotten the email. If not, see: https://reviews.llvm.org/D129749.
> By the way, if you have some spare time, you can also add the value for
> Musl (all 3 ABIs).  Musl has a different struct stat than Glibc (it's PR
> 106136 in GCC bugzilla).
I might be able to, but no promises. :)


From: Xi Ruoyao <xry111@xry111.site>
Sent: Wednesday, July 13, 2022 4:38 AM
To: Dimitrije Milosevic <Dimitrije.Milosevic@Syrmia.com>; Hans-Peter Nilsson <hp@bitrange.com>
Cc: Djordje Todorovic <Djordje.Todorovic@syrmia.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Subject: Re: Mips: Fix kernel_stat structure size 
 
On Tue, 2022-07-12 at 06:42 +0000, Dimitrije Milosevic wrote:

> I will send out a review, covering both 32 ABIs, meaning that both O32
> and N32 ABIs will be working properly.

Please CC me (@xry111) in LLVM review.

By the way, if you have some spare time, you can also add the value for
Musl (all 3 ABIs).  Musl has a different struct stat than Glibc (it's PR
106136 in GCC bugzilla).

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University