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

[Rainer Orth <ro at CeBiTec dot Uni-Bielefeld dot DE>]

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?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-12-04  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR testsuite/88208
	* gcc.target/sparc/attr-aligned.c (MAXALIGN) [__sparcv9 ||
	__arch64__]: Define.

# HG changeset patch
# Parent  bd6c4f1f5f5ef5a6d912a66d6f3b40f21c8dd411
Provide SPARCv9 MAXALIGN in gcc.target/sparc/attr-aligned.c

diff --git a/gcc/testsuite/gcc.target/sparc/attr-aligned.c b/gcc/testsuite/gcc.target/sparc/attr-aligned.c
--- a/gcc/testsuite/gcc.target/sparc/attr-aligned.c
+++ b/gcc/testsuite/gcc.target/sparc/attr-aligned.c
@@ -10,7 +10,11 @@
 #define HAS_ALIGN(f, n)  __builtin_has_attribute (f, __aligned__ (n))
 
 #define MINALIGN(N)   ((N) < 4 ? 4 : (N))
+#if defined(__sparcv9) || defined(__arch64__)
+#define MAXALIGN      16
+#else
 #define MAXALIGN      8
+#endif
 
 /* No alignment specified.  */
 void f (void) { }