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
