arm: Silence gcc warnings about arch ABI drift

  32-bit arm builds uniquely emit a lot of spam like this:

    fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
    fs/bcachefs/backpointers.c:15:13: note: parameter passing for argument of type ‘struct bch_backpointer’ changed in GCC 9.1

Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
warnings about arch ABI drift") to silence them. It seems like Dave's
original rationale applies here too.

Cc: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
---
 arch/arm/Makefile | 3 +++
 1 file changed, 3 insertions(+)
  

On Mon, Feb 19, 2024, at 05:09, Calvin Owens wrote:
> 32-bit arm builds uniquely emit a lot of spam like this:
>
>     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
>     fs/bcachefs/backpointers.c:15:13: note: parameter passing for 
> argument of type ‘struct bch_backpointer’ changed in GCC 9.1
>
> Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
> warnings about arch ABI drift") to silence them. It seems like Dave's
> original rationale applies here too.
>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> ---

I think these should be addressed in bcachefs instead.
While it's not the fault of bcachefs that the calling
convention changed between gcc versions, have a look at
the actual structure layout:

struct bch_val {
        __u64           __nothing[0];
};
struct bpos {
        /*
         * Word order matches machine byte order - btree code treats a bpos as a
         * single large integer, for search/comparison purposes
         *
         * Note that wherever a bpos is embedded in another on disk data
         * structure, it has to be byte swabbed when reading in metadata that
         * wasn't written in native endian order:
         */
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
        __u32           snapshot;
        __u64           offset;
        __u64           inode;
#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
        __u64           inode;
        __u64           offset;         /* Points to end of extent - sectors */
        __u32           snapshot;
#else
#error edit for your odd byteorder.
#endif
} __packed
struct bch_backpointer {
        struct bch_val          v;
        __u8                    btree_id;
        __u8                    level;
        __u8                    data_type;
        __u64                   bucket_offset:40;
        __u32                   bucket_len;
        struct bpos             pos;
} __packed __aligned(8);

This is not something that should ever be passed by value
into a function.

      Arnd
  

On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 19, 2024, at 05:09, Calvin Owens wrote:
> > 32-bit arm builds uniquely emit a lot of spam like this:
> >
> >     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
> >     fs/bcachefs/backpointers.c:15:13: note: parameter passing for 
> > argument of type ‘struct bch_backpointer’ changed in GCC 9.1
> >
> > Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
> > warnings about arch ABI drift") to silence them. It seems like Dave's
> > original rationale applies here too.
> >
> > Cc: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> > ---
> 
> I think these should be addressed in bcachefs instead.
> While it's not the fault of bcachefs that the calling
> convention changed between gcc versions, have a look at
> the actual structure layout:
> 
> struct bch_val {
>         __u64           __nothing[0];
> };
> struct bpos {
>         /*
>          * Word order matches machine byte order - btree code treats a bpos as a
>          * single large integer, for search/comparison purposes
>          *
>          * Note that wherever a bpos is embedded in another on disk data
>          * structure, it has to be byte swabbed when reading in metadata that
>          * wasn't written in native endian order:
>          */
> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>         __u32           snapshot;
>         __u64           offset;
>         __u64           inode;
> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>         __u64           inode;
>         __u64           offset;         /* Points to end of extent - sectors */
>         __u32           snapshot;
> #else
> #error edit for your odd byteorder.
> #endif
> } __packed
> struct bch_backpointer {
>         struct bch_val          v;
>         __u8                    btree_id;
>         __u8                    level;
>         __u8                    data_type;
>         __u64                   bucket_offset:40;
>         __u32                   bucket_len;
>         struct bpos             pos;
> } __packed __aligned(8);
> 
> This is not something that should ever be passed by value
> into a function.

Why?
  

On Monday 02/19 at 07:21 +0100, Arnd Bergmann wrote:
> On Mon, Feb 19, 2024, at 05:09, Calvin Owens wrote:
> > 32-bit arm builds uniquely emit a lot of spam like this:
> >
> >     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
> >     fs/bcachefs/backpointers.c:15:13: note: parameter passing for 
> > argument of type ‘struct bch_backpointer’ changed in GCC 9.1
> >
> > Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
> > warnings about arch ABI drift") to silence them. It seems like Dave's
> > original rationale applies here too.
> >
> > Cc: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> > ---
>
> I think these should be addressed in bcachefs instead.

That seems reasonable to me. For clarity, I just happened to notice this
while doing allyesconfig cross builds for something entirely unrelated.

I'll take it up with them. It's not a big problem from my POV, the notes
don't cause -Werror builds to fail or anything like that.

Thanks,
Calvin

> While it's not the fault of bcachefs that the calling
> convention changed between gcc versions, have a look at
> the actual structure layout:
> 
> struct bch_val {
>         __u64           __nothing[0];
> };
> struct bpos {
>         /*
>          * Word order matches machine byte order - btree code treats a bpos as a
>          * single large integer, for search/comparison purposes
>          *
>          * Note that wherever a bpos is embedded in another on disk data
>          * structure, it has to be byte swabbed when reading in metadata that
>          * wasn't written in native endian order:
>          */
> #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>         __u32           snapshot;
>         __u64           offset;
>         __u64           inode;
> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>         __u64           inode;
>         __u64           offset;         /* Points to end of extent - sectors */
>         __u32           snapshot;
> #else
> #error edit for your odd byteorder.
> #endif
> } __packed
> struct bch_backpointer {
>         struct bch_val          v;
>         __u8                    btree_id;
>         __u8                    level;
>         __u8                    data_type;
>         __u64                   bucket_offset:40;
>         __u32                   bucket_len;
>         struct bpos             pos;
> } __packed __aligned(8);
> 
> This is not something that should ever be passed by value
> into a function.
> 
>       Arnd
  

On Sun, Feb 18, 2024 at 10:58:07PM -0800, Calvin Owens wrote:
> On Monday 02/19 at 07:21 +0100, Arnd Bergmann wrote:
> > On Mon, Feb 19, 2024, at 05:09, Calvin Owens wrote:
> > > 32-bit arm builds uniquely emit a lot of spam like this:
> > >
> > >     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
> > >     fs/bcachefs/backpointers.c:15:13: note: parameter passing for 
> > > argument of type ‘struct bch_backpointer’ changed in GCC 9.1
> > >
> > > Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
> > > warnings about arch ABI drift") to silence them. It seems like Dave's
> > > original rationale applies here too.
> > >
> > > Cc: Dave Martin <Dave.Martin@arm.com>
> > > Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> > > ---
> >
> > I think these should be addressed in bcachefs instead.
> 
> That seems reasonable to me. For clarity, I just happened to notice this
> while doing allyesconfig cross builds for something entirely unrelated.
> 
> I'll take it up with them. It's not a big problem from my POV, the notes
> don't cause -Werror builds to fail or anything like that.

Considering we're not dynamic linking it's a non issue for us.
  

On Monday 02/19 at 02:03 -0500, Kent Overstreet wrote:
> On Sun, Feb 18, 2024 at 10:58:07PM -0800, Calvin Owens wrote:
> > On Monday 02/19 at 07:21 +0100, Arnd Bergmann wrote:
> > > On Mon, Feb 19, 2024, at 05:09, Calvin Owens wrote:
> > > > 32-bit arm builds uniquely emit a lot of spam like this:
> > > >
> > > >     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
> > > >     fs/bcachefs/backpointers.c:15:13: note: parameter passing for 
> > > > argument of type ‘struct bch_backpointer’ changed in GCC 9.1
> > > >
> > > > Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
> > > > warnings about arch ABI drift") to silence them. It seems like Dave's
> > > > original rationale applies here too.
> > > >
> > > > Cc: Dave Martin <Dave.Martin@arm.com>
> > > > Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> > > > ---
> > >
> > > I think these should be addressed in bcachefs instead.
> > 
> > That seems reasonable to me. For clarity, I just happened to notice this
> > while doing allyesconfig cross builds for something entirely unrelated.
> > 
> > I'll take it up with them. It's not a big problem from my POV, the notes
> > don't cause -Werror builds to fail or anything like that.
> 
> Considering we're not dynamic linking it's a non issue for us.

[ dropping arm people/lists ]

Would you mind taking this then?

Thanks,
Calvin

---8<---
From: Calvin Owens <jcalvinowens@gmail.com>
Subject: [PATCH] bcachefs: Silence gcc warnings about arm arch ABI drift

32-bit arm builds emit a lot of spam like this:

    fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
    fs/bcachefs/backpointers.c:15:13: note: parameter passing for argument of type ‘struct bch_backpointer’ changed in GCC 9.1

Apply the change from commit ebcc5928c5d9 ("arm64: Silence gcc warnings
about arch ABI drift") to fs/bcachefs/ to silence them.

Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
---
 fs/bcachefs/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/bcachefs/Makefile b/fs/bcachefs/Makefile
index 1a05cecda7cc..3433959d4f35 100644
--- a/fs/bcachefs/Makefile
+++ b/fs/bcachefs/Makefile
@@ -90,3 +90,6 @@ bcachefs-y		:=	\
 	xattr.o
 
 obj-$(CONFIG_MEAN_AND_VARIANCE_UNIT_TEST)   += mean_and_variance_test.o
+
+# Silence "note: xyz changed in GCC X.X" messages
+subdir-ccflags-y += $(call cc-disable-warning, psabi)
  

On Sun, Feb 18, 2024 at 11:36:08PM -0800, Calvin Owens wrote:
> On Monday 02/19 at 02:03 -0500, Kent Overstreet wrote:
> > On Sun, Feb 18, 2024 at 10:58:07PM -0800, Calvin Owens wrote:
> > > On Monday 02/19 at 07:21 +0100, Arnd Bergmann wrote:
> > > > On Mon, Feb 19, 2024, at 05:09, Calvin Owens wrote:
> > > > > 32-bit arm builds uniquely emit a lot of spam like this:
> > > > >
> > > > >     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
> > > > >     fs/bcachefs/backpointers.c:15:13: note: parameter passing for 
> > > > > argument of type ‘struct bch_backpointer’ changed in GCC 9.1
> > > > >
> > > > > Apply the arm64 change from commit ebcc5928c5d9 ("arm64: Silence gcc
> > > > > warnings about arch ABI drift") to silence them. It seems like Dave's
> > > > > original rationale applies here too.
> > > > >
> > > > > Cc: Dave Martin <Dave.Martin@arm.com>
> > > > > Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> > > > > ---
> > > >
> > > > I think these should be addressed in bcachefs instead.
> > > 
> > > That seems reasonable to me. For clarity, I just happened to notice this
> > > while doing allyesconfig cross builds for something entirely unrelated.
> > > 
> > > I'll take it up with them. It's not a big problem from my POV, the notes
> > > don't cause -Werror builds to fail or anything like that.
> > 
> > Considering we're not dynamic linking it's a non issue for us.
> 
> [ dropping arm people/lists ]
> 
> Would you mind taking this then?
> 
> Thanks,
> Calvin
 
That looks like just the thing - thanks!

> ---8<---
> From: Calvin Owens <jcalvinowens@gmail.com>
> Subject: [PATCH] bcachefs: Silence gcc warnings about arm arch ABI drift
> 
> 32-bit arm builds emit a lot of spam like this:
> 
>     fs/bcachefs/backpointers.c: In function ‘extent_matches_bp’:
>     fs/bcachefs/backpointers.c:15:13: note: parameter passing for argument of type ‘struct bch_backpointer’ changed in GCC 9.1
> 
> Apply the change from commit ebcc5928c5d9 ("arm64: Silence gcc warnings
> about arch ABI drift") to fs/bcachefs/ to silence them.
> 
> Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
> ---
>  fs/bcachefs/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/bcachefs/Makefile b/fs/bcachefs/Makefile
> index 1a05cecda7cc..3433959d4f35 100644
> --- a/fs/bcachefs/Makefile
> +++ b/fs/bcachefs/Makefile
> @@ -90,3 +90,6 @@ bcachefs-y		:=	\
>  	xattr.o
>  
>  obj-$(CONFIG_MEAN_AND_VARIANCE_UNIT_TEST)   += mean_and_variance_test.o
> +
> +# Silence "note: xyz changed in GCC X.X" messages
> +subdir-ccflags-y += $(call cc-disable-warning, psabi)
> -- 
> 2.43.0
>
  

On Mon, Feb 19, 2024, at 07:25, Kent Overstreet wrote:
> On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
>> 
>> This is not something that should ever be passed by value
>> into a function.
>
> Why?

Mostly because of the size, as 8 registers (on 32-bit) or
4 registers (on 64 bit) mean that even in the best case
passing these through argument registers is going to cause
spills if anything else gets passed as well. Passing them
through the stack in turn requires the same number of
registers to get copied in and out for every call, which
in turn can evict other registers that hold local variables.

On top of that, you have all the complications that make
passing them inconsistent and possibly worse depending
on how exactly a particular ABI passes structures:

- If each struct member is passed individually, you always
  need eight registers
- bitfields tend to get the compiler into corner cases
  that are not as optimized
- __u64 members tend to need even/odd register pairs on
  many 32-bit architectures.
- on big-endian kernels, two __u64 members are
  misaligned, which causes them to be in two halves
  of separate registers if the struct gets passed as
  four 64-bit regs.
  
I can't think of any platform where passing the structure
through a const pointer ends up worse than passing
by value.

    Arnd
  

On Mon, Feb 19, 2024 at 09:26:51AM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
> > I think these should be addressed in bcachefs instead.
> > While it's not the fault of bcachefs that the calling
> > convention changed between gcc versions, have a look at
> > the actual structure layout:
> > 
> > struct bch_val {
> >         __u64           __nothing[0];
> > };
> > struct bpos {
> >         /*
> >          * Word order matches machine byte order - btree code treats a bpos as a
> >          * single large integer, for search/comparison purposes
> >          *
> >          * Note that wherever a bpos is embedded in another on disk data
> >          * structure, it has to be byte swabbed when reading in metadata that
> >          * wasn't written in native endian order:
> >          */
> > #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >         __u32           snapshot;
> >         __u64           offset;
> >         __u64           inode;
> > #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >         __u64           inode;
> >         __u64           offset;         /* Points to end of extent - sectors */
> >         __u32           snapshot;
> > #else
> > #error edit for your odd byteorder.
> > #endif
> > } __packed
> > struct bch_backpointer {
> >         struct bch_val          v;
> >         __u8                    btree_id;
> >         __u8                    level;
> >         __u8                    data_type;
> >         __u64                   bucket_offset:40;
> >         __u32                   bucket_len;
> >         struct bpos             pos;
> > } __packed __aligned(8);
> > 
> > This is not something that should ever be passed by value
> > into a function.
> 
> +1 - bcachefs definitely needs fixing. Passing all that as an argument
> not only means that it has to be read into registers, but also when
> accessing members, it has to be extracted from those registers as well.
> 
> Passing that by argument is utterly insane.

If the compiler people can't figure out a vaguely efficient way to pass
a small struct by value, that's their problem - from the way you
describe it, I have to wonder at what insanity is going on there.
  

On Mon, Feb 19, 2024 at 09:52:08AM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 19, 2024 at 04:40:00AM -0500, Kent Overstreet wrote:
> > On Mon, Feb 19, 2024 at 09:26:51AM +0000, Russell King (Oracle) wrote:
> > > On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
> > > > I think these should be addressed in bcachefs instead.
> > > > While it's not the fault of bcachefs that the calling
> > > > convention changed between gcc versions, have a look at
> > > > the actual structure layout:
> > > > 
> > > > struct bch_val {
> > > >         __u64           __nothing[0];
> > > > };
> > > > struct bpos {
> > > >         /*
> > > >          * Word order matches machine byte order - btree code treats a bpos as a
> > > >          * single large integer, for search/comparison purposes
> > > >          *
> > > >          * Note that wherever a bpos is embedded in another on disk data
> > > >          * structure, it has to be byte swabbed when reading in metadata that
> > > >          * wasn't written in native endian order:
> > > >          */
> > > > #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > > >         __u32           snapshot;
> > > >         __u64           offset;
> > > >         __u64           inode;
> > > > #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > > >         __u64           inode;
> > > >         __u64           offset;         /* Points to end of extent - sectors */
> > > >         __u32           snapshot;
> > > > #else
> > > > #error edit for your odd byteorder.
> > > > #endif
> > > > } __packed
> > > > struct bch_backpointer {
> > > >         struct bch_val          v;
> > > >         __u8                    btree_id;
> > > >         __u8                    level;
> > > >         __u8                    data_type;
> > > >         __u64                   bucket_offset:40;
> > > >         __u32                   bucket_len;
> > > >         struct bpos             pos;
> > > > } __packed __aligned(8);
> > > > 
> > > > This is not something that should ever be passed by value
> > > > into a function.
> > > 
> > > +1 - bcachefs definitely needs fixing. Passing all that as an argument
> > > not only means that it has to be read into registers, but also when
> > > accessing members, it has to be extracted from those registers as well.
> > > 
> > > Passing that by argument is utterly insane.
> > 
> > If the compiler people can't figure out a vaguely efficient way to pass
> > a small struct by value, that's their problem - from the way you
> > describe it, I have to wonder at what insanity is going on there.
> 
> It sounds like you have a personal cruisade here.
> 
> Passing structures on through function arguments is never efficient.
> The entire thing _has_ to be loaded from memory at function call and
> either placed onto the stack (creating an effective memcpy() on every
> function call) or into registers. Fundamentally. It's not something
> compiler people can mess around with.
> 
> Sorry but it's bcachefs that's the problem here, and well done to the
> compiler people for pointing out stupid code.

Eh? Passing via stack copy is normal and expected; you were talking
about something else.

I'm not always trying to write code that will generate the fastest
assembly possible; there aro other considerations. As long a the
compiler is doing something /reasonable/, the code is fine.
  

On Mon, Feb 19, 2024, at 10:40, Kent Overstreet wrote:
> On Mon, Feb 19, 2024 at 09:26:51AM +0000, Russell King (Oracle) wrote:
>> On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
>> 
>> +1 - bcachefs definitely needs fixing. Passing all that as an argument
>> not only means that it has to be read into registers, but also when
>> accessing members, it has to be extracted from those registers as well.
>> 
>> Passing that by argument is utterly insane.
>
> If the compiler people can't figure out a vaguely efficient way to pass
> a small struct by value, that's their problem - from the way you
> describe it, I have to wonder at what insanity is going on there.

On most ABIs, there are only six argument registers (24 bytes)
for function calls. The compiler has very little choice here if
it tries to pass 32 bytes worth of data.

On both x86_64 and arm64, there are theoretically enough
registers to pass the data, but kernel disallows using the
vector and floating point registers for passing large
compounds arguments.

The integer registers on x86 apparently allow passing compounds
up to 16 bytes, but only if all members are naturally aligned.
Since you have both __packed members and bitfields, the compiler
would not even be allowed to pass the structure efficiently
even if it was small enough.

      Arnd
  

On Mon, Feb 19, 2024 at 10:57:46AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 19, 2024, at 10:40, Kent Overstreet wrote:
> > On Mon, Feb 19, 2024 at 09:26:51AM +0000, Russell King (Oracle) wrote:
> >> On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
> >> 
> >> +1 - bcachefs definitely needs fixing. Passing all that as an argument
> >> not only means that it has to be read into registers, but also when
> >> accessing members, it has to be extracted from those registers as well.
> >> 
> >> Passing that by argument is utterly insane.
> >
> > If the compiler people can't figure out a vaguely efficient way to pass
> > a small struct by value, that's their problem - from the way you
> > describe it, I have to wonder at what insanity is going on there.
> 
> On most ABIs, there are only six argument registers (24 bytes)
> for function calls. The compiler has very little choice here if
> it tries to pass 32 bytes worth of data.
> 
> On both x86_64 and arm64, there are theoretically enough
> registers to pass the data, but kernel disallows using the
> vector and floating point registers for passing large
> compounds arguments.
> 
> The integer registers on x86 apparently allow passing compounds
> up to 16 bytes, but only if all members are naturally aligned.
> Since you have both __packed members and bitfields, the compiler
> would not even be allowed to pass the structure efficiently
> even if it was small enough.

from an efficiency pov, the thing that matters is whether the
compiler is going to emit a call to memcpy (bad) or inline the copy -
and also whether the compiler can elide the copy if the variable is
never modified in the callee.

if were passing by reference it's also going to be living on the stack,
not registers.
  

On Mon, Feb 19, 2024 at 07:53:15PM +0000, David Laight wrote:
> ...
> > I'm not always trying to write code that will generate the fastest
> > assembly possible; there aro other considerations. As long a the
> > compiler is doing something /reasonable/, the code is fine.
> 
> Speaks the man who was writing horrid 'jit' code ...
> 
> This also begs the question of why that data is so compressed
> in the first place?
> It is quite likely that a few accesses generate far more code
> than the data you are attempting to save.

It's easy to understand if you understand profiling, benchmarking and
cache effects.

That 'jit code' is for _all_ filesystem metadata.