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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Mar 5, 2009 at 7:07 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> 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.
>

Even if we use 8 bit on off_align, there will be 24 unused bits.
There will be no change in the size of tree_decl_common., like

--- ./tree.h.pr39323    2009-03-03 10:20:55.000000000 -0800
+++ ./tree.h    2009-03-05 07:16:07.000000000 -0800
@@ -2726,13 +2726,17 @@ 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;
+  /* Padding so that 'off_align' can be on a 32-bit boundary.  */
+  unsigned decl_common_unused_1 : 2;

-  unsigned int align : 24;
   /* DECL_OFFSET_ALIGN, used only for FIELD_DECLs.  */
   unsigned int off_align : 8;

+  unsigned decl_common_unused_2 : 24;
+
+  /* DECL_ALIGN.  It should have the same size as TYPE_ALIGN.  */
+  unsigned int align;
+
   tree size_unit;
   tree initial;
   tree attributes;


-- 
H.J.