On 11/27/18 9:32 PM, Martin Sebor wrote: > 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. > > [*] See the following discussion for some background: > https://gcc.gnu.org/ml/gcc/2018-11/msg00127.html > > gcc-88208.diff > > PR testsuite/88208 - new test case c-c++-common/builtin-has-attribute-3.c in > r266335 has multiple excess errors > > gcc/ChangeLog: > PR testsuite/88208 > * doc/extend.texi (attribute constructor): Clarify. > > gcc/c/ChangeLog: > > PR testsuite/88208 > * c-decl.c (declspec_add_alignas): Adjust call to check_user_alignment. > > gcc/c-family/ChangeLog: > > PR testsuite/88208 > * c-attribs.c (common_handle_aligned_attribute): Silently avoid setting > alignments to values less than the target requires. > (has_attribute): For attribute aligned consider both the attribute > and the alignment bits. > * c-common.c (c_init_attributes): Optionally issue a warning for > zero alignment. > > gcc/testsuite/ChangeLog: > > PR testsuite/88208 > * gcc.dg/attr-aligned-2.c: New test. > * gcc.dg/builtin-has-attribute.c: Adjust. > * c-c++-common/builtin-has-attribute-2.c: Same. > * c-c++-common/builtin-has-attribute-3.c: Same. > * c-c++-common/builtin-has-attribute-4.c: Same. > * c-c++-common/builtin-has-attribute-5.c: New test. > * gcc.target/aarch64/attr-aligned.c: Same. > * gcc.target/i386/attr-aligned.c: Same. > * gcc.target/powerpc/attr-aligned.c: Same. > * gcc.target/sparc/attr-aligned.c: Same. > > Index: gcc/c/c-decl.c > =================================================================== > --- gcc/c/c-decl.c (revision 266521) > +++ gcc/c/c-decl.c (working copy) > @@ -11061,12 +11061,15 @@ struct c_declspecs * > declspecs_add_alignas (location_t loc, > struct c_declspecs *specs, tree align) > { > - int align_log; > specs->alignas_p = true; > specs->locations[cdw_alignas] = loc; > if (align == error_mark_node) > return specs; > - align_log = check_user_alignment (align, false, true); > + > + /* Only accept the alignment if it's valid and greater than > + the current one. Zere is invalid but by C11 required to be > + silently ignored. */ s/Zere/Zero/
OK with the nit fixed. jeff
