diff mbox

[c++] : Fix for PR/65390

Message ID CAEwic4aBYYmNx3CA6uuhsVe7LnSTqP2gnGJvM9y8k=NYEOaDgg@mail.gmail.com
State New
Headers show

Commit Message

Kai Tietz March 16, 2015, 1:50 p.m. UTC
Hi,

this patch avoids the attempt to create user-aligned-type for variants
original and main-variants type-alignment differ and original type
isn't user-aligned.
Not sure if this is the preferred variant, we could create for such
cases an aligned-type without setting user-align.
But as this should just happen for invalid types, I am not sure if we
actual need to handle later at all.
So I suggest the following fix for it:

ChangeLog

    PR c++/65390
    * tree.c (strip_typedefs): Don't attempt
    to build user-aligned type if original doesn't
    set it and main-variant differs to it.

Jonathan is just about to reduce the testcase, so final patch should
include reduced testcase for it.

Regression-tested for x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai

Comments

Jason Merrill March 16, 2015, 6:07 p.m. UTC | #1
If there is an alignment mismatch without user intervention, there is a 
problem, we can't just ignore it.

Where we run into trouble is with array types where the version built 
earlier has not been laid out yet but the new one has been.  I've been 
trying to deal with that by making sure that we lay out the original 
type as well, but obviously that isn't working for this case.  Why not?

I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN 
nor TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.

Jason
Kai Tietz March 16, 2015, 7:22 p.m. UTC | #2
2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
> If there is an alignment mismatch without user intervention, there is a
> problem, we can't just ignore it.
>
> Where we run into trouble is with array types where the version built
> earlier has not been laid out yet but the new one has been.  I've been
> trying to deal with that by making sure that we lay out the original type as
> well, but obviously that isn't working for this case.  Why not?

Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
(result) (value 8), and TYPE_USER_ALIGN isn't set.

> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN nor
> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.

For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
modified type).  So we could add here additional check if TYPE_SIZE is
a integer-constant?

Something like this condition you mean?

...
if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
    || ((TYPE_USER_ALIGN (t) || TREE_CODE (TYPE_SIZE (t)) == INTEGER_CST)
        && TYPE_ALIGN (t) != TYPE_ALIGN (result)))
  {
...
> Jason

Kai
Jason Merrill March 17, 2015, 12:36 p.m. UTC | #3
On 03/16/2015 03:22 PM, Kai Tietz wrote:
> 2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
>> If there is an alignment mismatch without user intervention, there is a
>> problem, we can't just ignore it.
>>
>> Where we run into trouble is with array types where the version built
>> earlier has not been laid out yet but the new one has been.  I've been
>> trying to deal with that by making sure that we lay out the original type as
>> well, but obviously that isn't working for this case.  Why not?
>
> Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
> (result) (value 8), and TYPE_USER_ALIGN isn't set.
>
>> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN nor
>> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.
>
> For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
> modified type).

TYPE_ALIGN should still be correct in that case.  So we need to figure 
out why result is getting the wrong alignment.

Jason
Kai Tietz March 17, 2015, 1:24 p.m. UTC | #4
2015-03-17 13:36 GMT+01:00 Jason Merrill <jason@redhat.com>:
> On 03/16/2015 03:22 PM, Kai Tietz wrote:
>>
>> 2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
>>>
>>> If there is an alignment mismatch without user intervention, there is a
>>> problem, we can't just ignore it.
>>>
>>> Where we run into trouble is with array types where the version built
>>> earlier has not been laid out yet but the new one has been.  I've been
>>> trying to deal with that by making sure that we lay out the original type
>>> as
>>> well, but obviously that isn't working for this case.  Why not?
>>
>>
>> Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
>> (result) (value 8), and TYPE_USER_ALIGN isn't set.
>>
>>> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN
>>> nor
>>> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.
>>
>>
>> For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
>> modified type).
>
>
> TYPE_ALIGN should still be correct in that case.  So we need to figure out
> why result is getting the wrong alignment.
>
> Jason
>

By debugging in build_cplus_array_type I see that type is marked as
dependent.  This is caused by type-max being an expression
non-constant.  So we later on don't layout this type.
So result isn't a comlete layout type.  by callling layout_type on
result, alignment fits.

Kai
diff mbox

Patch

Index: tree.c
===================================================================
--- tree.c      (Revision 221277)
+++ tree.c      (Arbeitskopie)
@@ -1356,7 +1356,7 @@  strip_typedefs (tree t)
   if (!result)
       result = TYPE_MAIN_VARIANT (t);
   if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
-      || TYPE_ALIGN (t) != TYPE_ALIGN (result))
+      || (TYPE_USER_ALIGN (t) && TYPE_ALIGN (t) != TYPE_ALIGN (result)))
     {
       gcc_assert (TYPE_USER_ALIGN (t));
       if (TYPE_ALIGN (t) == TYPE_ALIGN (result))