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

On 02/17/2016 12:39 AM, Jakub Jelinek wrote:
> 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.

Please see my note in the PS.

>> +#undef MAX_STACK_ALIGNMENT
>> +#define MAX_STACK_ALIGNMENT __UINT32_MAX__
>> +
>
> This is wrong for 2 reasons:

Doh! Sorry about that! This was leftover from an experiment I did
at one point and forgot to take out. (You do not need to explain
why it would be wrong to commit, it's obvious.)

>>     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;

Thanks. I haven't been able to cause the original code to misbehave
but I vaguely recall running into a problem (ICE?) with one of my
previous changes where I didn't check tree_fits_uhwi_p() in a compiler
built for a 32-bit host.  If that's possible it would be nice if this
trap could somehow be pointed out at compile time via a warning.

>
>> +	/* 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.

I shifted the alignment so that it could be compared against
MAX_STACK_ALIGNMENT.  But after some searching it seems as though
MAX_STACK_ALIGNMENT is in bits, rather than bytes as I had assumed,
so I've removed the shift.

(It would be helpful if the unit of MAX_STACK_ALIGNMENT was mentioned
in a comment in defaults.h where the macro is defined.)

With that cleared up, you seem to be suggesting that the alignment
argument shouldn't be checked against the macro.  I see
MAX_STACK_ALIGNMENT being used as the upper bound on stack alignment
in check_cxx_fundamental_alignment_constraints.   I also see that both
the C and C++ front ends reject stack-based variable declarations
aligned on a stricter boundary, such as:

   void foo (void) {
     _Alignas (1 << 29) int i;    // alignas in C++
   }

(Although the check for _Alignas and C++ alignas uses
(HOST_BITS_PER_INT - BITS_PER_UNIT_LOG)).

Is there a specific reason why you're advising against using
MAX_STACK_ALIGNMENT here?

> 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)

It's not obvious to me that this is guaranteed to be correct.
IMO, even if it happens to be, I find it much clearer to check
against MAX_STACK_ALIGNMENT (or whatever macro describes the
limit if not this one).

> 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.

That would be incorrect because ~0U isn't neither a power of 2, nor
the enforced stack alignment.  Again, using the actual limit encoded
in MAX_STACK_ALIGNMENT seems correct and IMO results in much clearer
code.

>> --- 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.

I don't mind waiting a bit for the documentation review, but I do
feel it's important to update the documentation at the same time
as making the change to the interface of the builtin.  The GCC
Coding Conventions even requires it:

   Any change to documented behavior (for example, the behavior of
   a command-line option or a GNU language extension) must include
   the necessary changes to the manual.

Attached is an updated patch.

Martin

PS I also feel that consistency is important, and I usually find
working with a consistently formatted code base far more enjoyable
(unless of course it comes at the expense of readability).

But since people have different personal preferences, unless the
preferred style is documented in the GCC style conventions or in
the or GNU coding standard (and ideally checked by an automated
tool like the check_GNU_style.sh script), some divergence is
unavoidable.

FWIW, since I haven't noticed a clear preference for either of
these two styles in the code base I decided to count the number
of occurrences of each to see if one is prevalent.  Although
the results are mildly in favor of the style you suggest, they
clearly indicate the lack of consensus:

$ find ~/scm/fsf/gcc-git/gcc ! -path "*/testsuite/*" \( -name "*.c" -o 
-name "*.h" \) | xargs grep "^[[:space:]][[:space:]]*=[^-+/*&!=]" | wc -l
5451

$ find ~/scm/fsf/gcc-git/gcc ! -path "*/testsuite/*" \( -name "*.c" -o 
-name "*.h" \) | xargs grep "[^-+/*&!=]=[[:space:]]*$" | wc -l
2000

In light of this, I see no point in spending time on trying to
adjust my changes or debating whether the equals looks better
on one line or the other.  There are far more important problems
to solve.

If I may make a suggestion: you would like to see the style
you prefer used consistently, bring it on the list to make
sure everyone knows about and agrees with it and then add it
to the GCC coding conventions and update check_GNU_style.sh
script to help enforce it.  Otherwise, trying to enforce it
in ad hoc code reviews is obviously ineffective and a waste
of everyone's time.  Not to mention that it's frustrating
to those of us who have made an effort to comply with the
documented style conventions and who try to follow the style
already in use to have our changes seemingly arbitrarily
picked on by different reviewers with each having their own
standard for the enforcement of the ideal formatting style.

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

Commit Message

Comments

Patch