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 PR testsuite/88208 * gcc.target/sparc/attr-aligned.c (MAXALIGN) [__sparcv9 || __arch64__]: Define.