[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[dberlin at gcc dot gnu dot org]

------- Comment #38 from dberlin at gcc dot gnu dot org  2005-10-04 12:30 -------
As a 4.1 kludge, i can make the points-to analyzer do what it does for unions,
which is to glob everything to a single variable for those classes where it has
found two fields it thinks overlap.

This will lose alias precision, but it won't be worse than what 4.0 was.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[pinskia at gcc dot gnu dot org]

------- Comment #39 from pinskia at gcc dot gnu dot org  2005-10-04 14:06 -------
*** Bug 24190 has been marked as a duplicate of this bug. ***


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Thomas dot Svedberg at
                   |                            |chalmers dot se


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[rguenth at gcc dot gnu dot org]

------- Comment #40 from rguenth at gcc dot gnu dot org  2005-10-04 15:12 -------
(In reply to comment #38)
> As a 4.1 kludge, i can make the points-to analyzer do what it does for unions,
> which is to glob everything to a single variable for those classes where it has
> found two fields it thinks overlap.
> 
> This will lose alias precision, but it won't be worse than what 4.0 was.

Yes please.  This bug really annoys me ;)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mark at codesourcery dot com]

------- Comment #41 from mark at codesourcery dot com  2005-10-04 15:28 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

rguenth at gcc dot gnu dot org wrote:
> ------- Comment #40 from rguenth at gcc dot gnu dot org  2005-10-04 15:12 -------
> (In reply to comment #38)
> 
>>As a 4.1 kludge, i can make the points-to analyzer do what it does for unions,
>>which is to glob everything to a single variable for those classes where it has
>>found two fields it thinks overlap.
>>
>>This will lose alias precision, but it won't be worse than what 4.0 was.
> 
> 
> Yes please.  This bug really annoys me ;)

I agree that this is a reasonable solution.  In fact, if you see
conflicting field types for some chunk of memory, I don't think it would
necessarily be unreasonable to just say that the whole object has some
unknown-type alias information.  C++ should provide an accurate
representation of multiple inheritance to the middle-end; if it doesn't,
it gets what it deserves, but it would be nice if programs still worked.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jason at redhat dot com]

------- Comment #42 from jason at redhat dot com  2005-10-04 21:05 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

mark at codesourcery dot com wrote:
> So, I guess maybe we agree on the right plan.  In particular:
> 
> 1. When we see "T*" or "T&", use the as-base version of T for the type.
>  Therefore, for expressions like "*p" where "p" has type "T*", we would
> give the expression the as-base version of T.  However, for expressions
> like "t" (where "t" is of type "T"), use the complete version of "t".

Yep.

> 2. Undo your change at the end of layout_class_type, so that base fields
> have the as-base type.

Agreed.

> We could implement (1) either by (a) doing that directly during parsing,
> etc., or (b) by handling it during genericization/gimplification.  I
> vote for the latter.  It should be relatively simple; do a pre-order
> walk of the expression tree, and adjust the types of things from the
> bottom up.
> 
> I'm not sure what else we would need to do.  We need to tell the
> optimizers that the as-base type can alias the complete type; I guess we
> just give them the same alias set.

I was suggesting that instead of making the complete type and base type 
essentially copies of one another, that we make the complete type a 
wrapper around the base type.  Proper alias semantics would fall out of 
that.

> We also need the optimizers to
> understand that "*p = *q" is going to copy TYPE_SIZE (complete type)
> bytes...

We don't do bitwise assignment if a class has a vptr, so whenever we 
generate something like that the complete type and base type should be 
the same.

Jason


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mark at codesourcery dot com]

------- Comment #43 from mark at codesourcery dot com  2005-10-04 21:14 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

jason at redhat dot com wrote:

> I was suggesting that instead of making the complete type and base type 
> essentially copies of one another, that we make the complete type a 
> wrapper around the base type.  Proper alias semantics would fall out of 
> that.

That's a nice idea.  It would make life a little more complicated for
the front end; when seeing "a.b", you have to generate a double
COMPONENT_REF, because the internal representation is a.as_base.b.  But,
that's a relatively localized change.  We could actually do that at
gimplification/genericization-time too, I guess, which would be a nice
start towards proper lowering.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[cvs-commit at gcc dot gnu dot org]

------- Comment #44 from cvs-commit at gcc dot gnu dot org  2005-10-06 19:38 -------
Subject: Bug 22488

CVSROOT:        /cvs/gcc
Module name:    gcc
Changes by:     dberlin@gcc.gnu.org     2005-10-06 19:38:00

Modified files:
        gcc            : ChangeLog tree-ssa-structalias.c 
Added files:
        gcc/testsuite/g++.dg/tree-ssa: pr22488.C 

Log message:
        2005-10-06  Daniel Berlin  <dberlin@dberlin.org>

        Fix PR tree-optimization/22488
        * tree-ssa-structalias.c (check_for_overlaps): New function.
        (create_variable_info_for): Use it.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.10106&r2=2.10107
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/tree-ssa-structalias.c.diff?cvsroot=gcc&r1=2.30&r2=2.31
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/tree-ssa/pr22488.C.diff?cvsroot=gcc&r1=NONE&r2=1.1


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[dberlin at gcc dot gnu dot org]

------- Comment #45 from dberlin at gcc dot gnu dot org  2005-10-08 17:26 -------
Pushing the real fix off to 4.2 and unassigning, since it is worked around for
4.1


-- 

dberlin at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|dberlin at gcc dot gnu dot  |unassigned at gcc dot gnu
                   |org                         |dot org
             Status|ASSIGNED                    |NEW
   Target Milestone|4.1.0                       |4.2.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[pinskia at gcc dot gnu dot org]

------- Comment #46 from pinskia at gcc dot gnu dot org  2005-10-09 15:45 -------
The real fix is a front-end fix.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |c++


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P5
   Target Milestone|4.2.0                       |4.1.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.1.0                       |4.2.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jason at gcc dot gnu dot org]

------- Comment #47 from jason at gcc dot gnu dot org  2006-08-22 21:54 -------
Accepting so I remember to implement my proposal sometime soon.


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |jason at gcc dot gnu dot org
                   |dot org                     |
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2006-01-30 12:40:45         |2006-08-22 21:54:36
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2/4.3 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mmitchel at gcc dot gnu dot org]

------- Comment #48 from mmitchel at gcc dot gnu dot org  2007-05-14 22:27 -------
Will not be fixed in 4.2.0; retargeting at 4.2.1.


-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.2.0                       |4.2.1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2/4.3 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mmitchel at gcc dot gnu dot org]

-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.2.1                       |4.2.2


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2/4.3 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mmitchel at gcc dot gnu dot org]

------- Comment #49 from mmitchel at gcc dot gnu dot org  2007-10-09 19:22 -------
Change target milestone to 4.2.3, as 4.2.2 has been released.


-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.2.2                       |4.2.3


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2/4.3 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jsm28 at gcc dot gnu dot org]

------- Comment #50 from jsm28 at gcc dot gnu dot org  2008-02-01 16:52 -------
4.2.3 is being released now, changing milestones of open bugs to 4.2.4.


-- 

jsm28 at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.2.3                       |4.2.4


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2/4.3/4.4 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jsm28 at gcc dot gnu dot org]

------- Comment #51 from jsm28 at gcc dot gnu dot org  2008-05-19 20:22 -------
4.2.4 is being released, changing milestones to 4.2.5.


-- 

jsm28 at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.2.4                       |4.2.5


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.1/4.2/4.3/4.4 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jason at gcc dot gnu dot org]

------- Comment #52 from jason at gcc dot gnu dot org  2008-05-19 20:39 -------
retargeting for 4.4.


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.2.5                       |4.4.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.2/4.3/4.4 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jsm28 at gcc dot gnu dot org]

------- Comment #53 from jsm28 at gcc dot gnu dot org  2008-07-04 22:45 -------
Closing 4.1 branch.


-- 

jsm28 at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[4.1/4.2/4.3/4.4 Regression]|[4.2/4.3/4.4 Regression]
                   |push_fields_onto_fieldstack |push_fields_onto_fieldstack
                   |calculates offset           |calculates offset
                   |incorrectly                 |incorrectly


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.2/4.3/4.4/4.5 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.4.0                       |4.4.1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.2/4.3/4.4/4.5 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.4.1                       |4.4.2


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.2/4.3/4.4/4.5 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.4.2                       |4.4.3


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.3/4.4/4.5 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.4.3                       |4.4.4


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug c++/22488] [4.3/4.4/4.5/4.6 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.4.4                       |4.4.5


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-09-29 18:20 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

jason at redhat dot com wrote:

>>I agree that using COMPONENT_REFs is good, but I think that the
>>FIELD_DECL for B should have the as-base type.  Then, when you actually
>>form the COMPONENT_REF, you should do the equivalent of:
>>
>>  *(complete B*)&c.b_base
>>
>>That will give the expression the correct type from the front-end's
>>point of view, but not result in inaccuracy with respect to the
>>TYPE_FIELDS for C.  Obviously, we need to make sure that B-as-base
>>complete-B are in the same alias set.
>  
> The problem is that when you do that kind of type punning the optimizers 
> get lost.

We may have to find some way to give them a compass. :-)

> I think it might actually work better to use the base type for most 
> expressions, and only use the complete type when we know the complete 
> type of the expression.

Yes, I agree.  The logically best thing would be that (possibly after
some kind of lowering pass), the C++ front-end would generate
expressions using the complete or as-base types, appropriately,
accurately reflecting what we knew about what was there.

> Within the compiler, we ought to be able to express the complete type as 
> a RECORD_TYPE containing fields of the as-base type and any vbases, so 
> that the relationship between them is clear and we shouldn't need to do 
> any ugly casting.  Of course, this would also be a significant amount of 
> work.

We almost do create the types as you suggest; we just don't use them for
expressions.  The as-base type is a copy of the complete type, before
the vbases have been added, so it looks almost like a prefix of the
complete type.  The "almost" is because your code changes the types of
the base fields in the complete type but not in the as-base type.

So, I guess maybe we agree on the right plan.  In particular:

1. When we see "T*" or "T&", use the as-base version of T for the type.
 Therefore, for expressions like "*p" where "p" has type "T*", we would
give the expression the as-base version of T.  However, for expressions
like "t" (where "t" is of type "T"), use the complete version of "t".

2. Undo your change at the end of layout_class_type, so that base fields
have the as-base type.

We could implement (1) either by (a) doing that directly during parsing,
etc., or (b) by handling it during genericization/gimplification.  I
vote for the latter.  It should be relatively simple; do a pre-order
walk of the expression tree, and adjust the types of things from the
bottom up.

I'm not sure what else we would need to do.  We need to tell the
optimizers that the as-base type can alias the complete type; I guess we
just give them the same alias set.  We also need the optimizers to
understand that "*p = *q" is going to copy TYPE_SIZE (complete type)
bytes...



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jason at redhat dot com]

------- Additional Comments From jason at redhat dot com  2005-09-29 17:19 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

mark at codesourcery dot com wrote:
> 
> What I meant by "lying" (an admittedly overly contentious choice of
> word) was that the field for B-in-C is marked as having the complete B
> type due to the code at the end of layout_class_type.  However, the A
> virtual base isn't located in that B; it's located after the data
> members of C.  So, the base field for B is incorrect.

Nah, "lying" is fair.  If you lie to the compiler, it will have its revenge.

> I agree that using COMPONENT_REFs is good, but I think that the
> FIELD_DECL for B should have the as-base type.  Then, when you actually
> form the COMPONENT_REF, you should do the equivalent of:
> 
>   *(complete B*)&c.b_base
> 
> That will give the expression the correct type from the front-end's
> point of view, but not result in inaccuracy with respect to the
> TYPE_FIELDS for C.  Obviously, we need to make sure that B-as-base
> complete-B are in the same alias set.

The problem is that when you do that kind of type punning the optimizers 
get lost.

> (It might be even better just to have the front end consider B-as-base
> and complete-B as the same type, so that the expression could have the
> more-accurate B-as-base type.  But, that would be a huge change to the
> front end, so I think we have no choice but to use the complete-B type
> for an expression like "*(B*)&c".)

I think it might actually work better to use the base type for most 
expressions, and only use the complete type when we know the complete 
type of the expression.

Within the compiler, we ought to be able to express the complete type as 
a RECORD_TYPE containing fields of the as-base type and any vbases, so 
that the relationship between them is clear and we shouldn't need to do 
any ugly casting.  Of course, this would also be a significant amount of 
work.

Jason


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-09-28 20:40 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

dberlin at dberlin dot org wrote:

>>>And besides, my approach isn't lying at all about this testcase; since 
>>>there's only one appearance of the virtual base, it appears exactly 
>>>where the field lists say it will.  Daniel agreed that the problem is in 
>>>his code.
>>
>>What I meant by "lying" (an admittedly overly contentious choice of
>>word) was that the field for B-in-C is marked as having the complete B
>>type due to the code at the end of layout_class_type.  However, the A
>>virtual base isn't located in that B; it's located after the data
>>members of C.  So, the base field for B is incorrect.
> 
> 
> If you give me guys a way to detect this case from the middle end, i'm
> happy to say "Don't walk this structure, it's not really there".
> 
> :)

I don't think there's anyway to do that reliably.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[dberlin at dberlin dot org]

------- Additional Comments From dberlin at gcc dot gnu dot org  2005-09-28 20:34 -------
Subject: Re:  [4.1 Regression]
	push_fields_onto_fieldstack calculates offset incorrectly

On Wed, 2005-09-28 at 20:29 +0000, mark at codesourcery dot com wrote:
> ------- Additional Comments From mark at codesourcery dot com  2005-09-28 20:28 -------
> Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
>  calculates offset incorrectly
> 
> jason at redhat dot com wrote:
> 
> > And besides, my approach isn't lying at all about this testcase; since 
> > there's only one appearance of the virtual base, it appears exactly 
> > where the field lists say it will.  Daniel agreed that the problem is in 
> > his code.
> 
> What I meant by "lying" (an admittedly overly contentious choice of
> word) was that the field for B-in-C is marked as having the complete B
> type due to the code at the end of layout_class_type.  However, the A
> virtual base isn't located in that B; it's located after the data
> members of C.  So, the base field for B is incorrect.

If you give me guys a way to detect this case from the middle end, i'm
happy to say "Don't walk this structure, it's not really there".

:)

> 
> I agree that using COMPONENT_REFs is good, but I think that the
> FIELD_DECL for B should have the as-base type.  Then, when you actually
> form the COMPONENT_REF, you should do the equivalent of:
> 
>   *(complete B*)&c.b_base
> 
> That will give the expression the correct type from the front-end's
> point of view, but not result in inaccuracy with respect to the
> TYPE_FIELDS for C.  Obviously, we need to make sure that B-as-base
> complete-B are in the same alias set.
> 
> (It might be even better just to have the front end consider B-as-base
> and complete-B as the same type, so that the expression could have the
> more-accurate B-as-base type.  But, that would be a huge change to the
> front end, so I think we have no choice but to use the complete-B type
> for an expression like "*(B*)&c".)
> 
> 
> 



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mark at codesourcery dot com]

------- Additional Comments From mark at codesourcery dot com  2005-09-28 20:28 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

jason at redhat dot com wrote:

> And besides, my approach isn't lying at all about this testcase; since 
> there's only one appearance of the virtual base, it appears exactly 
> where the field lists say it will.  Daniel agreed that the problem is in 
> his code.

What I meant by "lying" (an admittedly overly contentious choice of
word) was that the field for B-in-C is marked as having the complete B
type due to the code at the end of layout_class_type.  However, the A
virtual base isn't located in that B; it's located after the data
members of C.  So, the base field for B is incorrect.

I agree that using COMPONENT_REFs is good, but I think that the
FIELD_DECL for B should have the as-base type.  Then, when you actually
form the COMPONENT_REF, you should do the equivalent of:

  *(complete B*)&c.b_base

That will give the expression the correct type from the front-end's
point of view, but not result in inaccuracy with respect to the
TYPE_FIELDS for C.  Obviously, we need to make sure that B-as-base
complete-B are in the same alias set.

(It might be even better just to have the front end consider B-as-base
and complete-B as the same type, so that the expression could have the
more-accurate B-as-base type.  But, that would be a huge change to the
front end, so I think we have no choice but to use the complete-B type
for an expression like "*(B*)&c".)



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[dberlin at dberlin dot org]

------- Additional Comments From dberlin at gcc dot gnu dot org  2005-09-28 20:16 -------
Subject: Re:  [4.1 Regression]
	push_fields_onto_fieldstack calculates offset incorrectly

On Wed, 2005-09-28 at 19:39 +0000, jason at redhat dot com wrote:
> ------- Additional Comments From jason at redhat dot com  2005-09-28 19:39 -------
> Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
>  calculates offset incorrectly
> 
> mmitchel at gcc dot gnu dot org wrote:
> > I think this is a hack so that later, when we use the COMPONENT_REFs, we don't
> > have to cast from the as-base type back to the nominal C++ type.  (Of course,
> > this would also be cleaner with a real C++-lowering pass.)
> > 
> > However, I think the right thing is to have the cast; as you've discovered,
> > Jason's approach is lying about what's really there.  Jason?
> 
> My intent was to use COMPONENT_REFs where it's close enough to the 
> truth, and casts where it isn't.  The middle/back end produced 
> significantly better code for COMPONENT_REFs than casts when I made the 
> change, particularly with aliasing; I'm not sure if it's gotten any 
> better since then.
> 
> And besides, my approach isn't lying at all about this testcase; since 
> there's only one appearance of the virtual base, it appears exactly 
> where the field lists say it will.  Daniel agreed that the problem is in 
> his code.
> 

No, it actually isn't.
I sent mark a followup mail, that didn't get put into the bug report for
some reason.

The type looks like this:


Main type of var: C

First field of C: field of type B ( <field_decl 0x40212ebc D.1814
 type <record_type 0x40212170 B ... chain <field_decl 0x40212b24 x>)

   First field of B : _vptr.B ( <field_decl 0x40212450 _vptr.B ... chain
<type_decl 0x401eec98>)

   Second field of B: type_decl ( <type_decl 0x401eec98 B ...     chain
<field_decl 0x40212564 D.1781>)

   Third field of B: field of type A ( <field_decl 0x40212564 D.1781 ...
chain NULL)>

Second field of C: field of type X (<field_decl 0x40212b24 x
    type <record_type 0x4020e844 X ... chain <type_decl 0x401eee38 C>)

Third field of C: type_decl (<type_decl 0x401eee38 C ...     chain
<field_decl 0x4021605c D.1817>)

Fourth field of C: field of type A (<field_decl 0x4021605c D.1817
    type <record_type 0x4020e9b4 A .. chain NULL)

This causes us to walk type A twice (once as the third field of B, once
as the fourth field of C), and where we get the offsets wrong, since we
process the fields of type A twice.

I don't see why type A appears in type C except through type B
Is this because of virtual inheritance?

If so, are those fields of "A in B" really *there*, at those offsets
from the beginning of type B?

The layout you had posted in the bug matches the offsets when we process
the type A field of C, but not the type A field of type B.


IE type A appears in two places.
It's the first place that screws us up.




-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[jason at redhat dot com]

------- Additional Comments From jason at redhat dot com  2005-09-28 19:39 -------
Subject: Re:  [4.1 Regression] push_fields_onto_fieldstack
 calculates offset incorrectly

mmitchel at gcc dot gnu dot org wrote:
> I think this is a hack so that later, when we use the COMPONENT_REFs, we don't
> have to cast from the as-base type back to the nominal C++ type.  (Of course,
> this would also be cleaner with a real C++-lowering pass.)
> 
> However, I think the right thing is to have the cast; as you've discovered,
> Jason's approach is lying about what's really there.  Jason?

My intent was to use COMPONENT_REFs where it's close enough to the 
truth, and casts where it isn't.  The middle/back end produced 
significantly better code for COMPONENT_REFs than casts when I made the 
change, particularly with aliasing; I'm not sure if it's gotten any 
better since then.

And besides, my approach isn't lying at all about this testcase; since 
there's only one appearance of the virtual base, it appears exactly 
where the field lists say it will.  Daniel agreed that the problem is in 
his code.

Jason


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488

[Bug tree-optimization/22488] [4.1 Regression] push_fields_onto_fieldstack calculates offset incorrectly

[mmitchel at gcc dot gnu dot org]

------- Additional Comments From mmitchel at gcc dot gnu dot org  2005-09-28 15:57 -------
> If so, are those fields of "A in B" really *there*, at those offsets
> from the beginning of type B?

Sort of.  Because B would otherwise need a virtual table pointer, it just uses
the one in A.  So, there is a vptr at offset zero from the start of B.  However,
there's no actual A object at the same address as B because A is a virtual base.
 There's certainly no A::i at offset four from the start of B, for example.
 
> The layout you had posted in the bug matches the offsets when we process
> the type A field of C, but not the type A field of type B.

Ugh.

We create two versions of class types: the "complete" class type and the
"as-base" type.  (Look for CLASSTYPE_AS_BASE.)  The as-base type does not
include virtual bases, and the B field in C should have the B-as-base type, not
the complete B type.  The B-as-base type does not have the A parts.  In fact, if
you look at layout_class_type, and, in particular, build_base_field, you'll see
it does use the as-base type.

However, at the end of layout_class_type, we have:

  /* Now that we're done with layout, give the base fields the real types.  */

and that installs the complete type.  

Jason added this in 2004:

+ 2004-04-30  Jason Merrill  <jason@redhat.com>
+
+       Refer to base members using COMPONENT_REFs where possible.
+       * class.c (build_simple_base_path): New fn.
+       (build_base_path): Use it for non-virtual base references.
+       (layout_class_type): Change base fields to their real type
+       after layout is done.

I think this is a hack so that later, when we use the COMPONENT_REFs, we don't
have to cast from the as-base type back to the nominal C++ type.  (Of course,
this would also be cleaner with a real C++-lowering pass.)

However, I think the right thing is to have the cast; as you've discovered,
Jason's approach is lying about what's really there.  Jason?

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at redhat dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22488