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

[Jeff Law <law at redhat dot com>]

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