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

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2025/4/11 04:53, David Laight wrote:
> On Thu, 10 Apr 2025 07:56:45 +0800
> Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> 
>> Hi David,
>>
>> On 2025/4/10 02:52, David Laight wrote:
>>> On Tue,  8 Apr 2025 19:44:47 +0800
>>> Gao Xiang <hsiangkao@linux.alibaba.com> 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