Re: Mips: Fix kernel_stat structure size

[Hans-Peter Nilsson <hp@bitrange.com>]

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