Re: [PATCH v3 2/2] aarch64: Fix bit-field alignment in param passing [PR105549]

[Christophe Lyon <christophe.lyon@arm.com>]

Hi!


On 1/13/23 16:38, Jakub Jelinek wrote:
> On Wed, Jan 11, 2023 at 03:18:06PM +0100, Christophe Lyon via Gcc-patches wrote:
>> While working on enabling DFP for AArch64, I noticed new failures in
>> gcc.dg/compat/struct-layout-1.exp (t028) which were not actually
>> caused by DFP types handling. These tests are generated during 'make
>> check' and enabling DFP made generation different (not sure if new
>> non-DFP tests are generated, or if existing ones are generated
>> differently, the tests in question are huge and difficult to compare).
>>
>> Anyway, I reduced the problem to what I attach at the end of the new
>> gcc.target/aarch64/aapcs64/va_arg-17.c test and rewrote it in the same
>> scheme as other va_arg* AArch64 tests.  Richard Sandiford further
>> reduced this to a non-vararg function, added as a second testcase.
>>
>> This is a tough case mixing bit-fields and alignment, where
>> aarch64_function_arg_alignment did not follow what its descriptive
>> comment says: we want to use the natural alignment of the bit-field
>> type only if the user didn't reduce the alignment for the bit-field
>> itself.
>>
>> The patch also adds a comment and assert that would help someone who
>> has to look at this area again.
>>
>> The fix would be very small, except that this introduces a new ABI
>> break, and we have to warn about that.  Since this actually fixes a
>> problem introduced in GCC 9.1, we keep the old computation to detect
>> when we now behave differently.
>>
>> This patch adds two new tests (va_arg-17.c and
>> pr105549.c). va_arg-17.c contains the reduced offending testcase from
>> struct-layout-1.exp for reference.  We update some tests introduced by
>> the previous patch, where parameters with bit-fields and packed
>> attribute now emit a different warning.
> 
> I'm seeing
> +FAIL: g++.target/aarch64/bitfield-abi-warning-align16-O2.C scan-assembler-times and\\tw0, w1, 1 10
> +FAIL: g++.target/aarch64/bitfield-abi-warning-align32-O2.C scan-assembler-times and\\tw0, w1, 1 10
> +FAIL: g++.target/aarch64/bitfield-abi-warning-align8-O2.C scan-assembler-times and\\tw0, w0, 1 11
> +FAIL: g++.target/aarch64/bitfield-abi-warning-align8-O2.C scan-assembler-times and\\tw0, w1, 1 18
> +FAIL: gcc.target/aarch64/sve/pcs/struct_3_128.c -march=armv8.2-a+sve (internal compiler error: in aarch64_layout_arg, at config/aarch64/aarch64.cc:7696)
> +FAIL: gcc.target/aarch64/sve/pcs/struct_3_128.c -march=armv8.2-a+sve (test for excess errors)
> +FAIL: gcc.target/aarch64/sve/pcs/struct_3_256.c -march=armv8.2-a+sve (internal compiler error: in aarch64_layout_arg, at config/aarch64/aarch64.cc:7696)
> +FAIL: gcc.target/aarch64/sve/pcs/struct_3_256.c -march=armv8.2-a+sve (test for excess errors)
> +FAIL: gcc.target/aarch64/sve/pcs/struct_3_512.c -march=armv8.2-a+sve (internal compiler error: in aarch64_layout_arg, at config/aarch64/aarch64.cc:7696)
> +FAIL: gcc.target/aarch64/sve/pcs/struct_3_512.c -march=armv8.2-a+sve (test for excess errors)
> regressions with this change.
> 

Really deeply sorry for this :-(


> aarch64.cc:7696 is for me the newly added:
> 
>> +  gcc_assert (alignment <= 16 * BITS_PER_UNIT
>> +	      && (!alignment || abi_break < alignment)
>> +	      && (!abi_break_packed || alignment < abi_break_packed));
> 
> assert.
> Details in
> https://kojipkgs.fedoraproject.org//work/tasks/2857/96062857/build.log
> (configure line etc.), plus if you
> wget https://kojipkgs.fedoraproject.org//work/tasks/2857/96062857/build.log
> sed -n '/^begin /,/^end/p' build.log | uuencode > you get a compressed tarball with the testsuite *.log files.

Thanks I managed to download this (you meant uudecode rather than 
uuencode ;-) )

I see the scan-assembler-times are also failing in gcc.target, I guess 
you just forgot to paste them?

 From your other message, it seems you are building with stack-protector 
enabled by default, but I can't see that in the configure lines?

Indeed I just checked the generated code with/without 
-fstack-protector-all, and it obviously changes a lot, thus breaking the 
fragile scan-assembler directives. As you said, it's easy to avoid with 
-fno-stack-protector.

I'll check the problem with the assert.

Thanks and sorry,

Christophe

> 
> 	Jakub
>