Re: [PATCH 1/2] erofs: add __packed annotation to union(__le16..)

[Gao Xiang]

On 2025/4/11 04:53, David Laight wrote:
> On Thu, 10 Apr 2025 07:56:45 +0800
> Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx> wrote:
>
> > Hi David,
> >
> > On 2025/4/10 02:52, David Laight wrote:
> > > On Tue,  8 Apr 2025 19:44:47 +0800
> > > Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx> wrote:
> > >    
> > > > I'm unsure why they aren't 2 bytes in size only in arm-linux-gnueabi.
> > >
> > > IIRC one of the arm ABI aligns structures on 32 bit boundaries.
> >
> > Thanks for your reply, but I'm not sure if it's the issue.
>
> Twas a guess, the fragment in the patch doesn't look as though it
> will add padding.
>
> All tests I've tried generate a 2 byte union.
> But there might be something odd about the definition of __le16.
>
> Or the compiler is actually broken!

Sigh, I'm not sure, it's really a mess but I don't have
more time to look into that.

> > >    
> > > >


..

> > I doesn't work and will report
> >
> > In file included from <command-line>:
> > In function 'erofs_check_ondisk_layout_definitions',
> >       inlined from 'erofs_module_init' at ../fs/erofs/super.c:817:2:
> > ./../include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_332' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct erofs_inode_compact) != 32
> >     542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >         |
>
> Try with just __packed __aligned(2) on the union definition.
> That should overcome whatever brain-damage is causing the larger alignment,

Currently it works fine with `__packed` on the union definition,

do you suggest adding both `__packed` and `__aligned(2)`, may
I ask what's the benefit of `__aligned(2)`?

> > > I'd add a compile assert (of some form) on the size of the structure.
> >
> > you mean
> >
> > @@ -435,6 +435,7 @@ static inline void erofs_check_ondisk_layout_definitions(void)
> >           };
> >
> >           BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128);
> > +       BUILD_BUG_ON(sizeof(union erofs_inode_i_nb) != 2);
> >           BUILD_BUG_ON(sizeof(struct erofs_inode_compact) != 32);
>
> I'm sure there is one that you can put in the .h file itself.
> Might have to be Static_assert().
>
> > ?
> >
> >
> > ./../include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_332' declared with attribute error: BUILD_BUG_ON failed: sizeof(union erofs_inode_i_nb) != 2
> >     542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >         |                                      ^
> >
> > That doesn't work too.
>
> That it the root of the problem.
> I'd check with just a 'short' rather than the __le16.

.. sigh.. I have no more interest on this now due to lack
of time (my current employer doesn't allow me), I think
if there is no better ideas, let's keep the original patch
way to resolve arm compile issues...

Thanks,
Gao Xiang