This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] be more permissive about function alignments (PR 88208)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]