Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

[Alistair Francis <alistair23@gmail.com>]

On Mon, Aug 12, 2019 at 4:02 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Mon, 12 Aug 2019, Alistair Francis wrote:
>
> > > * The layout of struct stat and struct stat64 is identical, except that
> > > some bytes that are padding in struct stat serve as high parts of fields
> > > that are wider in struct stat64 (and thus have endian-dependent positions
> > > as determined by the __field64 macro in bits/stat.h).
> >
> > __ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
> > same thing. The __pad1 changes in size between the two structs as one
> > is a short and one is just an int. I don't see how they are identical.
>
> What file are you looking at, in what version of the source tree?

I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
definition then the one you are looking at.

>
> I'm looking at sysdeps/unix/sysv/linux/generic/bits/stat.h.  As far as I
> can tell, your patch series doesn't change that file.  That's the version
> used by RV64 and it should be used by RV32 (the headers installed for RV32
> and RV64 should be identical apart from the special cases of
> gnu/lib-names-*.h and gnu/stubs-*.h; if it isn't, that needs to be fixed).

It looks like __field64(__ino_t, __ino64_t, st_ino) will use type int
for RV32 (I don't think __USE_FILE_OFFSET64 is defined) which might
need some changes.

>
> In that file, __pad1 is of type __dev_t in both struct stat and struct
> stat64.

In the file you are looking at that is correct.

>
> > > If those types are 64-bit, you should not have padding around them in
> > > struct stat, so as to preserve the property that struct stat and struct
> > > stat64 have the same layout.  I suppose this means bits/stat.h needs to
> > > check further macros such as __OFF_T_MATCHES_OFF64_T.
> >
> > Changing the padding would fix the problem. Just to be clear is that
> > what you are suggesting?
>
> Yes.
>
> Either change the definition of __field64, or the uses of it.

Yep, in the file you are looking at that seems the best way to go.

>
> Changing the definition runs into the complication that it's used three
> times and each has its own macro to say whether the two types in question
> match, but you could always have a #error if some match and others don't.
>
> > > You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1.  And
> > > you'll need to make wordsize-32/overflow.h define trivial versions of the
> > > *_overflow functions in cases where the types match (this should be done
> > > in that file, rather than making an RV32-specific copy, to benefit future
> > > ports that make the same choices as RV32).
> >
> > I think I tested that and it didn't fix the problem so I dropped it.
> > I'll add the XSTAT_IS_XSTAT64 define back.
> >
> > I'm not sure what you mean by then the types match?
>
> off_t and off64_t match if they have the same size and alignment (if
> __OFF_T_MATCHES_OFF64_T is defined).  Likewise for the other pairs.
>
> If off_t and off64_t match, there is no need for the "buf->__st_size_pad
> == 0" check in stat_overflow (and once you've removed the padding in that
> case, that check won't even compile because __st_size_pad won't exist).
> Likewise for the other pairs.  In practice, you can probably just make
> that function return 0 if all three macros are defined, rather than
> allowing for arbitrary subsets of the types matching or not matching.

Ok, I think I understand. I'll update the patch series.

>
> Similar considerations apply to statfs_overflow if the types and padding
> used in struct statfs match those used in struct statfs64.  So apply
> similar considerations to the two versions of that structure.

Ok

Alistair

>
> You probably don't need to do anything special with lseek_overflow because
> the compiler should just optimize that away when off_t and loff_t are the
> same type.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com