Re: [PATCH] 69780 - [4.9/5/6 Regression] ICE on __builtin_alloca_with_align, with small alignment

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Feb 16, 2016 at 06:04:48PM -0700, Martin Sebor wrote:
> >Formatting.  = needs to be on the next line.
> 
> There are literally dozens of examples of this style in this file
> alone.  In one of the two instances of this style in this patch,
> moving the equals sign to the next line would force me to split
> the initializer expression over the next two lines to avoid
> exceeding the 80 character per line limit and make the code
> harder to read.  I also don't see the style you suggest mentioned
> in the GNU coding standard or in the GCC coding conventions.
> I would prefer to leave this detail to the discretion of the
> author.

Please change this, consistency is very much desirable.  Yes, there are
various formatting inconsistencies (which some people fix them up as they touch
the code), but that doesn't mean new inconsistencies should be introduced.

> --- gcc/c-family/c-common.c	(revision 233476)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -9816,8 +9816,41 @@ check_builtin_function_arguments (tree f
>        || DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
>      return true;
>  
> +#undef MAX_STACK_ALIGNMENT
> +#define MAX_STACK_ALIGNMENT __UINT32_MAX__
> +  

This is wrong for 2 reasons:
1) redefining a standard macro to something completely different
   (especially in such a huge file, it is quite possible one day somebody
   will want to use this macro in its original meaning and this define
   would break it)
2) using a GCC host macro makes the code unportable, not just to various
   vendor compilers, but also to any GCC < 4.5, or even to more recent
   GCC versions on various less frequently used hosts which don't define
   their UINT32_TYPE

>    switch (DECL_FUNCTION_CODE (fndecl))
>      {
> +    case BUILT_IN_ALLOCA_WITH_ALIGN:
> +      {
> +	/* Get the requested alignment (in bits) if it's a constant
> +	   integer expression.  */
> +	unsigned HOST_WIDE_INT align =
> +	  TREE_CODE (args[1]) == INTEGER_CST ? tree_to_uhwi (args[1]) : 0;

Besides formatting, not all INTEGER_CSTs fit into uhwi.
So, you should use instead
	unsigned HOST_WIDE_INT align
	  = tree_fits_uhwi_p (args[1]) : tree_to_uhwi (args[1]) : 0;

> +	/* Determine if the requested alignment is a power of 2 greater
> +	   than CHAR_BIT.  */
> +	if ((align & (align - 1)) == 0)
> +	  align >>= LOG2_BITS_PER_UNIT;
> +	else
> +	  align = 0;

Ugh, why the shifting?  The alignment is in bits, and the alignment in bits
must be a power of two, and the alignment in bits must fit into host
unsigned int.
Thus, instead of the above do just
	if ((align & (align - 1)) != 0)
	  align = 0;

	/* Reject invalid alignments.  */
	if (align < BITS_PER_UNIT || (unsigned int) align != align)
or better
	/* Reject invalid alignments.  */
	if ((align & (align - 1)) != 0
	    || align < BITS_PER_UNIT
	    || (unsigned int) align != align)
and then
	  {
	    error_at (EXPR_LOC_OR_LOC (args[1], input_location),
		      "second argument to function %qE must be a constant "
		      "integer power of 2 between %qi and %qu",
		      fndecl, BITS_PER_UNIT, ~0U);
Or if you don't like ~0U, you can use INTTYPE_MAXIMUM (unsigned int),
but for unsigned type it will do the same thing.
Though, of course, ~0U is not a power of two, and because we assume
on the host just about everywhere two's complement, you could use
(unsigned int) INTTYPE_MINIMUM (int) instead, which is the largest
power of 2 number one could use.

> --- gcc/doc/extend.texi	(revision 233476)
> +++ gcc/doc/extend.texi	(working copy)
> @@ -10144,6 +10144,8 @@ in the Cilk Plus language manual which c
>  @node Other Builtins
>  @section Other Built-in Functions Provided by GCC
>  @cindex built-in functions
> +@findex __builtin_alloca
> +@findex __builtin_alloca_with_align

I'd prefer not to mix the documentation patch with the bugfix, otherwise
the documentation might defer review of the bugfix or vice versa.

	Jakub