[PATCH] be more permissive about function alignments (PR 88208)
Wed Dec 5 16:05:00 GMT 2018
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.
>> 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?
I have done that in the past(*) but hardcoding target-specific
assumptions into a general test didn't feel right either given
the design of the test suite (separate target tests).
[*] see the tangle of #ifdefs in gcc/testsuite/gcc.dg/attr-aligned.c
> 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?
More information about the Gcc-patches