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 Wed, Feb 17, 2016 at 09:20:01PM -0700, Martin Sebor wrote:
> >The reason why MAX_STACK_ALIGNMENT is wrong is that on most targets
> >it is terribly small number (a couple of bytes usually), only i?86/x86_64 is
> >an exception, because it is the only target that supports dynamic stack
> >realignment.
> 
> I see. Thank you for the explanation.  I've confirmed it in
> an arm-eabi cross compiler where MAX_STACK_ALIGNMENT is 64.
> 
> What I still don't understand is why a user-specified alignment
> is being tested for inequality to MAX_STACK_ALIGNMENT in
> check_cxx_fundamental_alignment_constraints (the code whose
> example I followed):
> 
> 7765 #undef MAX_TARGET_FIELD_ALIGNMENT
> 7766 /* For stack variables, the target supports at most
> 7767 MAX_STACK_ALIGNMENT. */
> 7768 else if (decl_function_context (node) != NULL
> 7769 && requested_alignment > (max_align = MAX_STACK_ALIGNMENT))
> 7770 alignment_too_large_p = true;
> 
> That would then seem also wrong, although I haven't been able to
> trigger that code with a simple test case because the call to
> decl_function_context() always returns null, so maybe the code
> is never used.

Bet that code predates PR33721 2010 Richard's changes.
So indeed, it looks wrong now.

> I introduced the check for the upper bound because larger alignment
> values (1L << 32 and greater) also cause an ICE.

Sure, I've said that the alignment in bits is passed in unsigned int
argument, which is why I've suggested the (unsigned int) align != align
check.

> --- gcc/c-family/c-common.c	(revision 233476)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -9818,6 +9818,33 @@ check_builtin_function_arguments (tree f
>  
>    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_fits_uhwi_p (args[1]) ? tree_to_uhwi (args[1]) : 0;

This has still wrong formatting and unnecessary INTEGER_CST check
(tree_fits_uhwi_p checks that the argument is non-NULL INTEGER_CST and only then
checks the value).  The formatting is wrong, because the && would need to go
below TREE_CODE in this case.

> +	/* Determine if the requested alignment is a power of 2.  */
> +	if ((align & (align - 1)))
> +	  align = 0;
> +
> +	/* The maximum alignment in bits corresponding to the same
> +	   maximum in bytes enforced in check_user_alignment().  */
> +	unsigned maxalign = (UINT_MAX >> 1) + 1;
> +  
> +	/* Reject invalid alignments.  */
> +	if (align < BITS_PER_UNIT || maxalign < align)
> +	  {
> +	    error_at (EXPR_LOC_OR_LOC (args [1], input_location),

No space before [.

> +		      "second argument to function %qE must be a constant "
> +		      "integer power of 2 between %qi and %qu bits",
> +		      fndecl, BITS_PER_UNIT, maxalign);
> +	    return false;
> +	  }
> +      return true;
> +      }

This looks ok to me.

The non-doc/ part of the changes are ok for trunk with the above mentioned
changes, the doc/ part I'll leave to Joseph or others, really not sure what
exactly we want to document and in what form.

	Jakub