This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] be more permissive about function alignments (PR 88208)
- From: Jeff Law <law at redhat dot com>
- To: Rainer Orth <ro at CeBiTec dot Uni-Bielefeld dot DE>, Martin Sebor <msebor at gmail dot com>
- Cc: Gcc Patch List <gcc-patches at gcc dot gnu dot org>, Eric Botcazou <ebotcazou at adacore dot com>
- Date: Wed, 5 Dec 2018 06:50:40 -0700
- Subject: Re: [PATCH] be more permissive about function alignments (PR 88208)
- References: <19112bc8-83a2-2c35-2841-d95087cd1178@gmail.com> <yddk1ko86i1.fsf@CeBiTec.Uni-Bielefeld.DE>
On 12/5/18 6:09 AM, Rainer Orth wrote:
> Hi Martin,
>
>> The tests for the new __builtin_has_attribute function have been
>> failing on a number of targets because of a couple of assumptions
>> that only hold on some.
>>
>> First, they expect that it's safe to apply attribute aligned with
>> a smaller alignment than the target provides when GCC rejects such
>> arguments. The tests pass on i86 and elsewhere but fail on
>> strictly aligned targets like aarch64 or sparc. After some testing
>> and thinking I don't think this is helpful -- I believe it's better
>> to instead silently accept attributes that ask for a less restrictive
>> alignment than the function ultimately ends up with (see * below).
>> This is what testing shows Clang does on those targets. The attached
>> patch implements this change.
>>
>> Second, the tests assume that the priority forms of the constructor
>> and destructor attributes are universally supported. That's also
>> not the case, even though the manual doesn't mention that. To
>> avoid these failures the attached patch moves the priority forms
>> of the attribute constructor and destructor tests into its own
>> file that's compiled only for init_priority targets.
>>
>> Finally, I noticed that attribute aligned accepts zero as
>> an argument even though it's not a power of two as the manual
>> documents as a precondition (zero is treated the same as
>> the attribute without an argument). A zero argument is likely
>> to be a mistake, especially when the zero comes from macro
>> expansion, that users might want to know about. Clang rejects
>> a zero with an error but I think a warning is more in line with
>> established GCC practice, so the patch also implements that.
>>
>> Besides x86_64-linux, I tested this change with cross-compilers
>> for aarch64-linux-elf, powerpc64le-linux, and sparc-solaris2.11.
>> I added tests for the changed aligned attribute for those targets
>> To make the gcc.dg/builtin-has-attribute.c test pass with
>> the cross-compilers I changed from a runtime test into a compile
>> only one.
>>
>> Martin
>>
>> PS I'm not happy about duplicating the same test across all those
>> targets. It would be much nicer to have a single test somewhere
>> in dg.exp #include a target-specific header with macros describing
>> the target-specific parameters.
>
> why so complicated? Just have a single attr-aligned.c test, restricted
> to the target cpus it supports via dg directives and with
> MINALIGN/MAXALIGN definitions controlled by appropriate target macros?
>
> Besides, the new gcc.target/sparc/attr-aligned.c test currently FAILs on
> 64-bit sparc:
>
> FAIL: gcc.target/sparc/attr-aligned.c (test for excess errors)
>
> Excess errors:
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/sparc/attr-aligned.c:29:1: error: static assertion failed: "alignof (f_) == MAXALIGN"
>
> alignof (f_) is 16 for sparcv9. The following patch fixes this, tested
> on sparc-sun-solaris2.11 with -m32 and -m64.
>
> Ok for mainline?
OK
jeff