Bug 44596 - [OOP] Dynamic dispatch uses broken types
Summary: [OOP] Dynamic dispatch uses broken types
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: ---
Assignee: Paul Thomas
URL:
Keywords: wrong-code
: 44745 44746 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-19 19:07 UTC by Richard Biener
Modified: 2010-07-13 10:01 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-07-01 18:27:06


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2010-06-19 19:07:55 UTC
On mem-ref2 branch gfortran.dg/dynamic_dispatch_6.f03 is miscompiled due
to TBAA issues as the Frontend uses two completely unrelated structure
types to access the same vtable pointer.

That of course can't work.
Comment 1 Richard Biener 2010-06-19 19:16:48 UTC
  static struct vtype$periodic_5th_factory vtab$periodic_5th_factory = {.$hash=9935896, .$size=0, .$extends=&vtab$field_factory};

...

  if (vtab$periodic_5th_factory.create == 0B)
    {
      vtab$periodic_5th_factory.create = new_periodic_5th_order;
    }
  field_creator.$vptr = (struct vtype$field_factory *) &vtab$periodic_5th_factory;
  u = VIEW_CONVERT_EXPR<struct class$field_p>(field_creator.$vptr->create (&field_creator));

is broken.  You access struct vtype$periodic_5th_factory via a pointer
of type struct vtype$field_factory but they are not in any way related.
Look into alias.c:record_component_aliases and provide proper BINFOs,
or if that is not possible make sure to disable TBAA for vtables
by making all pointers to them ref-all.
Comment 2 Tobias Burnus 2010-06-22 15:00:48 UTC
CC Paul as he might have an idea how to handle it (cf. comment 1).

BTW: The mem-ref2 branch (cf. http://gcc.gnu.org/wiki/MemRef ) will be merged soon.
Comment 3 paul.richard.thomas@gmail.com 2010-06-22 19:15:34 UTC
Subject: Re:  [OOP] Dynamic dispatch uses broken types

Dear Tobias,

> ------- Comment #2 from burnus at gcc dot gnu dot org  2010-06-22 15:00 -------
> CC Paul as he might have an idea how to handle it (cf. comment 1).

It seems that our attempt to use the front end to describe
polymorphism is running into difficulties and is facing us with what I
had hoped to avoid:  Coming to grips with the way that g++ does
things.  The reason that I had hoped to avoid this is obvious - none
of us have the time to do it.

Here is some partial documentation of what Richard is talking about:

http://www.delorie.com/gnu/docs/gcc/gccint_40.html

http://docs.freebsd.org/info/gxxint/gxxint.info.Macros.html

http://www.math.utah.edu/docs/info/gxxint_1.html

(some repetition).

On the other hand, surely our front-endery can be persuaded to work?
I guess that we have to ensure that the inherited vtables are all of
the same type as the base type.  Since all the vtables in a class
consist of procedure pointers and pointers to generic vtables with
exactly the same structure, this should be straightforward.  I guess
that the error is to add to the vtable all the extra methods of the
inherited type, rather than sticking to overloading.

> BTW: The mem-ref2 branch (cf. http://gcc.gnu.org/wiki/MemRef ) will be merged
> soon.

Oh bother, I feel another rush coming on, just when I was hoping that
we could take stock of OOP and consider how to get it right :-(

Paul
Comment 4 paul.richard.thomas@gmail.com 2010-06-23 17:36:07 UTC
Subject: Re:  [OOP] Dynamic dispatch uses broken types

Tobias,

On Tue, Jun 22, 2010 at 5:00 PM, burnus at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #2 from burnus at gcc dot gnu dot org  2010-06-22 15:00 -------
> CC Paul as he might have an idea how to handle it (cf. comment 1).

It's "obvious", the vtables for a given class can all be of the same
type, thus eliminating the problem. :-)
>
> BTW: The mem-ref2 branch (cf. http://gcc.gnu.org/wiki/MemRef ) will be merged
> soon.
>

Cheers

Paul
Comment 5 Richard Biener 2010-07-01 14:21:44 UTC
*** Bug 44745 has been marked as a duplicate of this bug. ***
Comment 6 Richard Biener 2010-07-01 14:22:11 UTC
*** Bug 44746 has been marked as a duplicate of this bug. ***
Comment 7 Dominique d'Humieres 2010-07-01 14:25:37 UTC
This may be a duplicate of pr44662. Could you try the patch in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44662#c2 ?
Comment 8 Richard Biener 2010-07-01 14:34:36 UTC
(In reply to comment #7)
> This may be a duplicate of pr44662. Could you try the patch in
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44662#c2 ?

It is not.
Comment 9 Dominique d'Humieres 2010-07-01 15:57:45 UTC
(In reply to comment #8)
> > This may be a duplicate of pr44662. Could you try the patch in
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44662#c2 ?
>
> It is not.

Agreed for this pr (and pr44745 is a duplicate). However I think pr44746 is a duplicate of pr44596 and should be fixed by the patch in its comment #2.
Comment 10 Paul Thomas 2010-07-01 18:27:06 UTC
(In reply to comment #9)
> (In reply to comment #8)

I'm on the road for a few days - I'll update the tree on my laptop and see what I can do :-)

Cheers

Paul
Comment 11 Paul Thomas 2010-07-04 14:40:48 UTC
Subject: Bug 44596

Author: pault
Date: Sun Jul  4 14:40:34 2010
New Revision: 161801

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161801
Log:
2010-07-04  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/44596
	PR fortran/44745
	* trans-types.c (gfc_get_derived_type): Derived type fields
	with the vtype attribute must have TYPE_REF_CAN_ALIAS_ALL set.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-types.c

Comment 12 Richard Biener 2010-07-05 12:32:39 UTC
 	field_type = build_pointer_type (field_type);
 
+      /* vtype fields can point to different types to the base type.  */
+      if (c->ts.type == BT_DERIVED && c->ts.u.derived->attr.vtype)
+	TYPE_REF_CAN_ALIAS_ALL (field_type) = true;
+

this is wrong.  You shouldn't change bits in (shared) types.  Instead do

        field_type = build_pointer_type_for_mode
            (field_type, ptr_mode,
             c->ts.type == BT_DERIVED && c->ts.u.derived->attr.vtype);
Comment 13 Tobias Burnus 2010-07-05 13:14:10 UTC
(In reply to comment #12)
> You shouldn't change bits in (shared) types.  Instead do
> 
>         field_type = build_pointer_type_for_mode
>             (field_type, ptr_mode,
>              c->ts.type == BT_DERIVED && c->ts.u.derived->attr.vtype);

Thanks for checking, Richard!


For those wondering, like me, where ptr_mode is defined: gcc/machmode.h has:
  extern enum machine_mode ptr_mode;

The the function itself is (tree.c):

/* Construct, lay out and return the type of pointers to TO_TYPE with
   mode MODE.  If CAN_ALIAS_ALL is TRUE, indicate this type can
   reference all of memory. If such a type has already been
   constructed, reuse it.  */

tree
build_pointer_type_for_mode (tree to_type, enum machine_mode mode,
                             bool can_alias_all)
Comment 14 paul.richard.thomas@gmail.com 2010-07-05 13:52:23 UTC
Subject: Re:  [OOP] Dynamic dispatch uses broken types

On Mon, Jul 5, 2010 at 3:14 PM, burnus at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:

> Thanks for checking, Richard!

Indeed, seconded by me.

Is there anywhere that these rather more subtle aspects of gcc are
documented in a structured way?  I am afraid that I just do not have
the time to do the archeology that seems to be necessary.

I'll do the necessary asap.

Thanks

Paul
Comment 15 rguenther@suse.de 2010-07-05 14:06:41 UTC
Subject: Re:  [OOP] Dynamic dispatch uses broken types

On Mon, 5 Jul 2010, paul dot richard dot thomas at gmail dot com wrote:

> ------- Comment #14 from paul dot richard dot thomas at gmail dot com  2010-07-05 13:52 -------
> Subject: Re:  [OOP] Dynamic dispatch uses broken types
> 
> On Mon, Jul 5, 2010 at 3:14 PM, burnus at gcc dot gnu dot org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> 
> > Thanks for checking, Richard!
> 
> Indeed, seconded by me.
> 
> Is there anywhere that these rather more subtle aspects of gcc are
> documented in a structured way?  I am afraid that I just do not have
> the time to do the archeology that seems to be necessary.

Well, it's the general invariant that all TYPEs and DECLs are
shared, likewise some constants (notably INTEGER_CSTs).
You should never modify them directly (unless you know
exactly what you are doing), but use construction functions from tree.c

> I'll do the necessary asap.

Thanks.
Comment 16 paul.richard.thomas@gmail.com 2010-07-05 15:47:04 UTC
Subject: Re:  [OOP] Dynamic dispatch uses broken types

> ------- Comment #15 from rguenther at suse dot de  2010-07-05 14:06 -------

Now I take a look at build_pointer_type_for_mode and it's uses in
gfortran, it seems rather obvious.  Thanks for the helping hand.

Paul
Comment 17 Paul Thomas 2010-07-05 19:26:25 UTC
Subject: Bug 44596

Author: pault
Date: Mon Jul  5 19:26:12 2010
New Revision: 161848

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161848
Log:
2010-07-05  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/44596
	* trans-types.c (gfc_get_derived_type): Derived type fields
	with the vtype attribute must have TYPE_REF_CAN_ALIAS_ALL set
	but build_pointer_type_for_mode must be used for this.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-types.c

Comment 18 Tobias Burnus 2010-07-06 15:20:32 UTC
Paul, thanks for the check in. Do you plan to backport it to 4.5, which sems to use the same code?
Comment 19 paul.richard.thomas@gmail.com 2010-07-06 15:28:27 UTC
Subject: Re:  [OOP] Dynamic dispatch uses broken types

Dear Tobias,

> Paul, thanks for the check in. Do you plan to backport it to 4.5, which sems to
> use the same code?

Yes, I could do that on Thursday, when I am back in Barcelona.  I do
not have 4.5 on the laptop.

Cheers

Paul
Comment 20 Paul Thomas 2010-07-11 17:45:01 UTC
(In reply to comment #19)
> Subject: Re:  [OOP] Dynamic dispatch uses broken types
> 
> Dear Tobias,
> 
> > Paul, thanks for the check in. Do you plan to backport it to 4.5, which sems to
> > use the same code?

When I add the vtype attribute and the above patch, almost every OOP test fails with, for example:

/svn/gcc-4_5-branch/gcc/testsuite/gfortran.dg/class_6.f03:7:0: internal compiler error: in gimplify_expr, at gimplify.c:7346
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

I think that, since we do not need this (yet!), I am disinclined to spend any more time on it, unless Richard understands what is happening.

Paul
Comment 21 Tobias Burnus 2010-07-13 10:01:04 UTC
(In reply to comment #19)
> > Paul, thanks for the check in. Do you plan to backport it to 4.5, which
> > sems to use the same code?
> 
> Yes, I could do that on Thursday, when I am back in Barcelona.  I do
> not have 4.5 on the laptop.
[...]
> When I add the vtype attribute and the above patch, almost every OOP test
> fails

Well, if it does not want to be fixed then we don't fix it. I had hoped that it would be trivial - which turned out to be not the case.

OOP was announced as experimental (in 4.5) - thus, OOP users can easily jump directly to 4.6.

> I think that, since we do not need this (yet!)

Then close it as FIXED (on the 4.6 trunk).