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?

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?

Thanks!
Martin


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