Re: [4.5] PATCH: PR c/39323: MAX_OFILE_ALIGNMENT in elfos.h is too big

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Mar 5, 2009 at 4:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> - Show quoted text -
> On Wed, Mar 4, 2009 at 12:36 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Mar 3, 2009 at 8:39 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Mon, Mar 2, 2009 at 10:27 PM, Ian Lance Taylor <iant@google.com> wrote:
>>>> "H.J. Lu" <hjl.tools@gmail.com> writes:
>>>>
>>>>> gcc/
>>>>>
>>>>> 2009-02-28 ?H.J. Lu ?<hongjiu.lu@intel.com>
>>>>>
>>>>> ? ? ? PR c/39323
>>>>> ? ? ? * c-common.c (handle_aligned_attribute): Check alignment
>>>>> ? ? ? overlow.
>>>>>
>>>>> gcc/testsuite/
>>>>>
>>>>> 2009-02-28 ?H.J. Lu ?<hongjiu.lu@intel.com>
>>>>>
>>>>> ? ? ? PR c/39323
>>>>> ? ? ? * gcc.dg/pr39323.c: New.
>>>>>
>>>>
>>>>> @@ -5954,7 +5956,19 @@ handle_aligned_attribute (tree *node, tr
>>>>> ? ? ? *type = build_variant_type_copy (*type);
>>>>
>>>>> ? ? ? ?TYPE_ALIGN (*type) = (1 << i) * BITS_PER_UNIT;
>>>>> - ? ? ?TYPE_USER_ALIGN (*type) = 1;
>>>>> + ? ? ?/* TYPE_ALIGN and DECL_ALIGN, defined as decl_common.align, may
>>>>> + ? ? ?have different widths. ?Use decl_common.align directly to check
>>>>> + ? ? ?for overflow. ?*/
>>>>> + ? ? ?saved = (*type)->decl_common.align;
>>>>> + ? ? ?(*type)->decl_common.align = (1 << i) * BITS_PER_UNIT;
>>>>> + ? ? ?if ((*type)->decl_common.align != (1 << i) * BITS_PER_UNIT)
>>>>> + ? ? {
>>>>> + ? ? ? error ("requested alignment is too large");
>>>>> + ? ? ? *no_add_attrs = true;
>>>>> + ? ? }
>>>>> + ? ? ?else
>>>>> + ? ? TYPE_USER_ALIGN (*type) = 1;
>>>>> + ? ? ?(*type)->decl_common.align = saved;
>>>>
>>>> It is never OK to refer to decl_common outside of tree.h. ?In any case I
>>>> don't see why it matters here whether decl_common.align overflows. ?What
>>>> matters is whether TYPE_ALIGN overflows.
>>>>
>>>> If you are concerned about the TYPE_ALIGN value overflowing when you
>>>> copy it to DECL_ALIGN, check it at the point of the copy to DECL_ALIGN.
>>>>
>>>> Please use a variable to hold the expression "(1 << i) * BIT_PER_UNIT",
>>>> rather than repeating it.
>>>>
>>>>> @@ -5977,7 +5991,13 @@ handle_aligned_attribute (tree *node, tr
>>>>> ? ?else
>>>>> ? ? ?{
>>>>> ? ? ? ?DECL_ALIGN (decl) = (1 << i) * BITS_PER_UNIT;
>>>>> - ? ? ?DECL_USER_ALIGN (decl) = 1;
>>>>> + ? ? ?if (DECL_ALIGN (decl) != (1 << i) * BITS_PER_UNIT)
>>>>> + ? ? {
>>>>> + ? ? ? error ("requested alignment is too large");
>>>>> + ? ? ? *no_add_attrs = true;
>>>>> + ? ? }
>>>>> + ? ? ?else
>>>>> + ? ? DECL_USER_ALIGN (decl) = 1;
>>>>
>>>> Please use a variable for "(1 << i) * BIT_PER_UNIT".
>>>>
>>>
>>> Here is the updated patch. It added decl_align_ok in
>>> tree.h.
>>>
>>> There are
>>>
>>> coverage.c: ? ? ?DECL_ALIGN (tree_ctr_tables[counter]) = TYPE_ALIGN
>>> (gcov_type_node);
>>> omp-low.c: ? ?DECL_ALIGN (field) = TYPE_ALIGN (type);
>>> stmt.c: ? ? ?DECL_ALIGN (decl) = TYPE_ALIGN (type);
>>> stor-layout.c: ? ? ?DECL_ALIGN (decl) = TYPE_ALIGN (type);
>>> tree-nested.c: ? ?TYPE_ALIGN (type) = DECL_ALIGN (field);
>>> tree-nested.c: ? ?DECL_ALIGN (field) = TYPE_ALIGN (TREE_TYPE (field));
>>> tree-nested.c: ? ? ?DECL_ALIGN (field) = TYPE_ALIGN (type);
>>> tree-nested.c: ? ? ?DECL_ALIGN (field) = TYPE_ALIGN (type);
>>> tree-vect-analyze.c: ? ? ?DECL_ALIGN (base) = TYPE_ALIGN (vectype);
>>> tree-vectorizer.c: ? ? ? ?DECL_ALIGN (decl) = TYPE_ALIGN (vectype);
>>> cp/class.c: ?DECL_ALIGN (decl) = MAX (TYPE_ALIGN (double_type_node),
>>> cp/class.c: ? ? ? DECL_ALIGN (decl) = CLASSTYPE_ALIGN (basetype);
>>> cp/class.c: ? ? ? DECL_ALIGN (field) = TYPE_ALIGN (integer_type);
>>> fortran/trans-common.c: ? ? DECL_ALIGN (decl) = TYPE_ALIGN (TREE_TYPE (field));
>>> java/typeck.c: ?DECL_ALIGN (arfld) = TYPE_ALIGN (element_type);
>>> ada/gcc-interface/utils.c: ? ? ?DECL_ALIGN (field_decl) = TYPE_ALIGN (field_type);
>>>
>>> If we want to delay DECL_ALIGN overflow check, we may need to
>>> check it at those places above and we may only call sorry ().
>>> I can prepare such a patch if it is desirable.
>>
>> Huhm. ?Instead of all this hassle why not make decl_common.align
>> unsigned int just like type.align? ?It can pack with pointer_alias_set
>> which is int as well, so make it
>>
>> ...
>> ?/* Padding so that 'align' can be on a 32-bit boundary. ?*/
>> ?unsigned decl_common_unused : 2;
>>
>> ?/* DECL_OFFSET_ALIGN, used only for FIELD_DECLs. ?*/
>> ?unsigned int off_align : 8;
>>
>> ?unsigned int align;
>>
>> ?alias_set_type pointer_alias_set;
>>
>> ?tree size_unit;
>> ...
>
> We will need another 32bit for this, like
>
> --- ./tree.h.pr39323 ? ?2009-02-26 10:59:38.000000000 -0800
> +++ ./tree.h ? ?2009-03-05 06:57:24.000000000 -0800
> @@ -2728,12 +2728,13 @@ struct tree_decl_common GTY(())
> ? unsigned gimple_reg_flag : 1;
> ? /* In a DECL with pointer type, set if no TBAA should be done. ?*/
> ? unsigned no_tbaa_flag : 1;
> - ?/* Padding so that 'align' can be on a 32-bit boundary. ?*/
> ? unsigned decl_common_unused : 2;
>
> - ?unsigned int align : 24;
> - ?/* DECL_OFFSET_ALIGN, used only for FIELD_DECLs. ?*/
> - ?unsigned int off_align : 8;
> + ?/* DECL_ALIGN. ?It should have the same size as TYPE_ALIGN. ?*/
> + ?unsigned int align;
> + ?/* DECL_OFFSET_ALIGN, used only for FIELD_DECLs. ?We don't need
> + ? ? 32bits for this. ?It can be reduced to 8 bits if necessary. ?*/
> + ?unsigned int off_align;
>
> ? tree size_unit;
> ? tree initial;

I don't see that DECL_OFFSET_ALIGN needs to be int as well.
DECL_OFFSET_ALIGN is a very special and strange beast.

Richard.

>
>
> --
> H.J.
>