diff mbox

[PR,44535] Fix a problem with folding OBJ_TYPE_REFs

Message ID 20100616162034.GB19794@virgil.suse.cz
State New
Headers show

Commit Message

Martin Jambor June 16, 2010, 4:20 p.m. UTC 
Hi,

as the testcase shows, my assumption that BASE_BINFOs were reordered
in a way that if any of the had virtual functions the first one did
too was false.  I do not really remember why I thought this, I must
have misunderstood results of some experiments I did last autumn.

The following patch fixes the folding by skipping BASE_BINFOs that do
not have virtual functions when looking for the "first" one.

In addition to the request for an approval of the patch, I also have
one question.  I believe it would be quite desirable to have a sanity
check in gimple_fold_obj_type_ref_known_binfo asserting that
BINFO_FLAG_2 is true.  In in the c++ front end this bit is referred to
as BINFO_NEW_VTABLE_MARKED.  Should I move the definition of this
macro from gcc/cp/cp-tree.h to gcc/tree.h so that we can use it for
the assert too?  Would such a patch be accepted?

As usual, I have bootstrapped and tested the patch on x86_64-linux, OK
for trunk?

Thanks,

Martin


2010-06-16  Martin Jambor  <mjambor@suse.cz>

	PR c++/44535
	* gimple-fold.c (get_first_base_binfo_with_virtuals): New function.
	(gimple_get_relevant_ref_binfo): Use get_first_base_binfo_with_virtuals
	instead of BINFO_BASE_BINFO.

	* testsuite/g++.dg/torture/pr44535.C: New file.

Comments

Richard Biener June 16, 2010, 4:34 p.m. UTC | #1 
On Wed, 16 Jun 2010, Martin Jambor wrote:

> Hi,
> 
> as the testcase shows, my assumption that BASE_BINFOs were reordered
> in a way that if any of the had virtual functions the first one did
> too was false.  I do not really remember why I thought this, I must
> have misunderstood results of some experiments I did last autumn.
> 
> The following patch fixes the folding by skipping BASE_BINFOs that do
> not have virtual functions when looking for the "first" one.
> 
> In addition to the request for an approval of the patch, I also have
> one question.  I believe it would be quite desirable to have a sanity
> check in gimple_fold_obj_type_ref_known_binfo asserting that
> BINFO_FLAG_2 is true.  In in the c++ front end this bit is referred to
> as BINFO_NEW_VTABLE_MARKED.  Should I move the definition of this
> macro from gcc/cp/cp-tree.h to gcc/tree.h so that we can use it for
> the assert too?  Would such a patch be accepted?

What's the semantic of that flag?

> As usual, I have bootstrapped and tested the patch on x86_64-linux, OK
> for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2010-06-16  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR c++/44535
> 	* gimple-fold.c (get_first_base_binfo_with_virtuals): New function.
> 	(gimple_get_relevant_ref_binfo): Use get_first_base_binfo_with_virtuals
> 	instead of BINFO_BASE_BINFO.
> 
> 	* testsuite/g++.dg/torture/pr44535.C: New file.
> 
> Index: icln/gcc/gimple-fold.c
> ===================================================================
> --- icln.orig/gcc/gimple-fold.c
> +++ icln/gcc/gimple-fold.c
> @@ -1403,6 +1403,22 @@ gimple_fold_builtin (gimple stmt)
>    return result;
>  }
>  
> +/* Return the first of the base binfos of BINFO that has virtual functions.  */
> +
> +static tree
> +get_first_base_binfo_with_virtuals (tree binfo)
> +{
> +  int i;
> +  tree base_binfo;
> +
> +  for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> +    if (BINFO_VIRTUALS (base_binfo))
> +      return base_binfo;
> +
> +  return NULL_TREE;
> +}
> +
> +
>  /* Search for a base binfo of BINFO that corresponds to TYPE and return it if
>     it is found or NULL_TREE if it is not.  */
>  
> @@ -1458,8 +1474,8 @@ gimple_get_relevant_ref_binfo (tree ref,
>  	      || BINFO_N_BASE_BINFOS (binfo) == 0)
>  	    return NULL_TREE;
>  
> -	  base_binfo = BINFO_BASE_BINFO (binfo, 0);
> -	  if (BINFO_TYPE (base_binfo) != TREE_TYPE (field))
> +	  base_binfo = get_first_base_binfo_with_virtuals (binfo);
> +	  if (base_binfo && BINFO_TYPE (base_binfo) != TREE_TYPE (field))
>  	    {
>  	      tree d_binfo;
>  
> Index: icln/gcc/testsuite/g++.dg/torture/pr44535.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/torture/pr44535.C
> @@ -0,0 +1,34 @@
> +/* { dg-do run } */
> +
> +namespace FOO {
> +
> +template <typename T>
> +class A
> +{
> +public:
> +    void Enum();
> +    virtual void OnProv() = 0;
> +    virtual ~A() { }
> +};
> +typedef A<char> B;
> +
> +template<typename T>
> +void A<T>::Enum ()
> +{
> +    OnProv ();
> +}
> +} // namespace FOO
> +
> +class C {};
> +
> +class D: public C, public FOO::B {
> +public:
> +    void OnProv() {}
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +    D x;
> +    x.Enum();
> +    return 0;
> +}
> 
>
Martin Jambor June 17, 2010, 1:07 p.m. UTC | #2 
Hi,

On Wed, Jun 16, 2010 at 06:34:21PM +0200, Richard Guenther wrote:
> On Wed, 16 Jun 2010, Martin Jambor wrote:
> 
> > Hi,
> > 
> > as the testcase shows, my assumption that BASE_BINFOs were reordered
> > in a way that if any of the had virtual functions the first one did
> > too was false.  I do not really remember why I thought this, I must
> > have misunderstood results of some experiments I did last autumn.
> > 
> > The following patch fixes the folding by skipping BASE_BINFOs that do
> > not have virtual functions when looking for the "first" one.
> > 
> > In addition to the request for an approval of the patch, I also have
> > one question.  I believe it would be quite desirable to have a sanity
> > check in gimple_fold_obj_type_ref_known_binfo asserting that
> > BINFO_FLAG_2 is true.  In in the c++ front end this bit is referred to
> > as BINFO_NEW_VTABLE_MARKED.  Should I move the definition of this
> > macro from gcc/cp/cp-tree.h to gcc/tree.h so that we can use it for
> > the assert too?  Would such a patch be accepted?
> 
> What's the semantic of that flag?

From what I can see in the cp front-end source, this flag is set when
a binfo gets its own virtual table - by which I mean the
BINFO_VIRTUALS list.  Before that it may share that list with the
binfo describing the standalone ancestor but that is of course wrong
for those methods that have been overridden in the descendant.  This
is what happens for BASE_BINFOs that are the first ones with virtual
functions (the vtable delta is zero) and therefore I treat them
specially in my folding code.

Even though I do believe that with this patch the code is now correct,
it would be nice to check that I have nevertheless not landed on a
binfo that does not have a vtable of its own, if only to be a bit more
robust when checking future changes (and avoid quite nasty miscompiles).

Naturally, I'll be much happier if some C++ maintainer actually
confirmed my observations are correct.

Thanks,

Martin

> 
> > As usual, I have bootstrapped and tested the patch on x86_64-linux, OK
> > for trunk?
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2010-06-16  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR c++/44535
> > 	* gimple-fold.c (get_first_base_binfo_with_virtuals): New function.
> > 	(gimple_get_relevant_ref_binfo): Use get_first_base_binfo_with_virtuals
> > 	instead of BINFO_BASE_BINFO.
> > 
> > 	* testsuite/g++.dg/torture/pr44535.C: New file.
> > 
> > Index: icln/gcc/gimple-fold.c
> > ===================================================================
> > --- icln.orig/gcc/gimple-fold.c
> > +++ icln/gcc/gimple-fold.c
> > @@ -1403,6 +1403,22 @@ gimple_fold_builtin (gimple stmt)
> >    return result;
> >  }
> >  
> > +/* Return the first of the base binfos of BINFO that has virtual functions.  */
> > +
> > +static tree
> > +get_first_base_binfo_with_virtuals (tree binfo)
> > +{
> > +  int i;
> > +  tree base_binfo;
> > +
> > +  for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> > +    if (BINFO_VIRTUALS (base_binfo))
> > +      return base_binfo;
> > +
> > +  return NULL_TREE;
> > +}
> > +
> > +
> >  /* Search for a base binfo of BINFO that corresponds to TYPE and return it if
> >     it is found or NULL_TREE if it is not.  */
> >  
> > @@ -1458,8 +1474,8 @@ gimple_get_relevant_ref_binfo (tree ref,
> >  	      || BINFO_N_BASE_BINFOS (binfo) == 0)
> >  	    return NULL_TREE;
> >  
> > -	  base_binfo = BINFO_BASE_BINFO (binfo, 0);
> > -	  if (BINFO_TYPE (base_binfo) != TREE_TYPE (field))
> > +	  base_binfo = get_first_base_binfo_with_virtuals (binfo);
> > +	  if (base_binfo && BINFO_TYPE (base_binfo) != TREE_TYPE (field))
> >  	    {
> >  	      tree d_binfo;
> >  
> > Index: icln/gcc/testsuite/g++.dg/torture/pr44535.C
> > ===================================================================
> > --- /dev/null
> > +++ icln/gcc/testsuite/g++.dg/torture/pr44535.C
> > @@ -0,0 +1,34 @@
> > +/* { dg-do run } */
> > +
> > +namespace FOO {
> > +
> > +template <typename T>
> > +class A
> > +{
> > +public:
> > +    void Enum();
> > +    virtual void OnProv() = 0;
> > +    virtual ~A() { }
> > +};
> > +typedef A<char> B;
> > +
> > +template<typename T>
> > +void A<T>::Enum ()
> > +{
> > +    OnProv ();
> > +}
> > +} // namespace FOO
> > +
> > +class C {};
> > +
> > +class D: public C, public FOO::B {
> > +public:
> > +    void OnProv() {}
> > +};
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +    D x;
> > +    x.Enum();
> > +    return 0;
> > +}
> > 
> > 
> 
> -- 
> Richard Guenther <rguenther@suse.de>
> Novell / SUSE Labs
> SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex
Martin Jambor June 28, 2010, 12:32 p.m. UTC | #3 
Ping.  Thanks,

Martin


On Wed, Jun 16, 2010 at 06:20:34PM +0200, Martin Jambor wrote:
> Hi,
> 
> as the testcase shows, my assumption that BASE_BINFOs were reordered
> in a way that if any of the had virtual functions the first one did
> too was false.  I do not really remember why I thought this, I must
> have misunderstood results of some experiments I did last autumn.
> 
> The following patch fixes the folding by skipping BASE_BINFOs that do
> not have virtual functions when looking for the "first" one.
> 
> In addition to the request for an approval of the patch, I also have
> one question.  I believe it would be quite desirable to have a sanity
> check in gimple_fold_obj_type_ref_known_binfo asserting that
> BINFO_FLAG_2 is true.  In in the c++ front end this bit is referred to
> as BINFO_NEW_VTABLE_MARKED.  Should I move the definition of this
> macro from gcc/cp/cp-tree.h to gcc/tree.h so that we can use it for
> the assert too?  Would such a patch be accepted?
> 
> As usual, I have bootstrapped and tested the patch on x86_64-linux, OK
> for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2010-06-16  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR c++/44535
> 	* gimple-fold.c (get_first_base_binfo_with_virtuals): New function.
> 	(gimple_get_relevant_ref_binfo): Use get_first_base_binfo_with_virtuals
> 	instead of BINFO_BASE_BINFO.
> 
> 	* testsuite/g++.dg/torture/pr44535.C: New file.
> 
> Index: icln/gcc/gimple-fold.c
> ===================================================================
> --- icln.orig/gcc/gimple-fold.c
> +++ icln/gcc/gimple-fold.c
> @@ -1403,6 +1403,22 @@ gimple_fold_builtin (gimple stmt)
>    return result;
>  }
>  
> +/* Return the first of the base binfos of BINFO that has virtual functions.  */
> +
> +static tree
> +get_first_base_binfo_with_virtuals (tree binfo)
> +{
> +  int i;
> +  tree base_binfo;
> +
> +  for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> +    if (BINFO_VIRTUALS (base_binfo))
> +      return base_binfo;
> +
> +  return NULL_TREE;
> +}
> +
> +
>  /* Search for a base binfo of BINFO that corresponds to TYPE and return it if
>     it is found or NULL_TREE if it is not.  */
>  
> @@ -1458,8 +1474,8 @@ gimple_get_relevant_ref_binfo (tree ref,
>  	      || BINFO_N_BASE_BINFOS (binfo) == 0)
>  	    return NULL_TREE;
>  
> -	  base_binfo = BINFO_BASE_BINFO (binfo, 0);
> -	  if (BINFO_TYPE (base_binfo) != TREE_TYPE (field))
> +	  base_binfo = get_first_base_binfo_with_virtuals (binfo);
> +	  if (base_binfo && BINFO_TYPE (base_binfo) != TREE_TYPE (field))
>  	    {
>  	      tree d_binfo;
>  
> Index: icln/gcc/testsuite/g++.dg/torture/pr44535.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/torture/pr44535.C
> @@ -0,0 +1,34 @@
> +/* { dg-do run } */
> +
> +namespace FOO {
> +
> +template <typename T>
> +class A
> +{
> +public:
> +    void Enum();
> +    virtual void OnProv() = 0;
> +    virtual ~A() { }
> +};
> +typedef A<char> B;
> +
> +template<typename T>
> +void A<T>::Enum ()
> +{
> +    OnProv ();
> +}
> +} // namespace FOO
> +
> +class C {};
> +
> +class D: public C, public FOO::B {
> +public:
> +    void OnProv() {}
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +    D x;
> +    x.Enum();
> +    return 0;
> +}
>
Richard Biener June 28, 2010, 12:39 p.m. UTC | #4 
On Mon, 28 Jun 2010, Martin Jambor wrote:

> Ping.  Thanks,

Ok.

Thanks,
Richard.

> Martin
> 
> 
> On Wed, Jun 16, 2010 at 06:20:34PM +0200, Martin Jambor wrote:
> > Hi,
> > 
> > as the testcase shows, my assumption that BASE_BINFOs were reordered
> > in a way that if any of the had virtual functions the first one did
> > too was false.  I do not really remember why I thought this, I must
> > have misunderstood results of some experiments I did last autumn.
> > 
> > The following patch fixes the folding by skipping BASE_BINFOs that do
> > not have virtual functions when looking for the "first" one.
> > 
> > In addition to the request for an approval of the patch, I also have
> > one question.  I believe it would be quite desirable to have a sanity
> > check in gimple_fold_obj_type_ref_known_binfo asserting that
> > BINFO_FLAG_2 is true.  In in the c++ front end this bit is referred to
> > as BINFO_NEW_VTABLE_MARKED.  Should I move the definition of this
> > macro from gcc/cp/cp-tree.h to gcc/tree.h so that we can use it for
> > the assert too?  Would such a patch be accepted?
> > 
> > As usual, I have bootstrapped and tested the patch on x86_64-linux, OK
> > for trunk?
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2010-06-16  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR c++/44535
> > 	* gimple-fold.c (get_first_base_binfo_with_virtuals): New function.
> > 	(gimple_get_relevant_ref_binfo): Use get_first_base_binfo_with_virtuals
> > 	instead of BINFO_BASE_BINFO.
> > 
> > 	* testsuite/g++.dg/torture/pr44535.C: New file.
> > 
> > Index: icln/gcc/gimple-fold.c
> > ===================================================================
> > --- icln.orig/gcc/gimple-fold.c
> > +++ icln/gcc/gimple-fold.c
> > @@ -1403,6 +1403,22 @@ gimple_fold_builtin (gimple stmt)
> >    return result;
> >  }
> >  
> > +/* Return the first of the base binfos of BINFO that has virtual functions.  */
> > +
> > +static tree
> > +get_first_base_binfo_with_virtuals (tree binfo)
> > +{
> > +  int i;
> > +  tree base_binfo;
> > +
> > +  for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> > +    if (BINFO_VIRTUALS (base_binfo))
> > +      return base_binfo;
> > +
> > +  return NULL_TREE;
> > +}
> > +
> > +
> >  /* Search for a base binfo of BINFO that corresponds to TYPE and return it if
> >     it is found or NULL_TREE if it is not.  */
> >  
> > @@ -1458,8 +1474,8 @@ gimple_get_relevant_ref_binfo (tree ref,
> >  	      || BINFO_N_BASE_BINFOS (binfo) == 0)
> >  	    return NULL_TREE;
> >  
> > -	  base_binfo = BINFO_BASE_BINFO (binfo, 0);
> > -	  if (BINFO_TYPE (base_binfo) != TREE_TYPE (field))
> > +	  base_binfo = get_first_base_binfo_with_virtuals (binfo);
> > +	  if (base_binfo && BINFO_TYPE (base_binfo) != TREE_TYPE (field))
> >  	    {
> >  	      tree d_binfo;
> >  
> > Index: icln/gcc/testsuite/g++.dg/torture/pr44535.C
> > ===================================================================
> > --- /dev/null
> > +++ icln/gcc/testsuite/g++.dg/torture/pr44535.C
> > @@ -0,0 +1,34 @@
> > +/* { dg-do run } */
> > +
> > +namespace FOO {
> > +
> > +template <typename T>
> > +class A
> > +{
> > +public:
> > +    void Enum();
> > +    virtual void OnProv() = 0;
> > +    virtual ~A() { }
> > +};
> > +typedef A<char> B;
> > +
> > +template<typename T>
> > +void A<T>::Enum ()
> > +{
> > +    OnProv ();
> > +}
> > +} // namespace FOO
> > +
> > +class C {};
> > +
> > +class D: public C, public FOO::B {
> > +public:
> > +    void OnProv() {}
> > +};
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +    D x;
> > +    x.Enum();
> > +    return 0;
> > +}
> > 
> 
>
diff mbox

Patch

Index: icln/gcc/gimple-fold.c
===================================================================
--- icln.orig/gcc/gimple-fold.c
+++ icln/gcc/gimple-fold.c
@@ -1403,6 +1403,22 @@  gimple_fold_builtin (gimple stmt)
   return result;
 }
 
+/* Return the first of the base binfos of BINFO that has virtual functions.  */
+
+static tree
+get_first_base_binfo_with_virtuals (tree binfo)
+{
+  int i;
+  tree base_binfo;
+
+  for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
+    if (BINFO_VIRTUALS (base_binfo))
+      return base_binfo;
+
+  return NULL_TREE;
+}
+
+
 /* Search for a base binfo of BINFO that corresponds to TYPE and return it if
    it is found or NULL_TREE if it is not.  */
 
@@ -1458,8 +1474,8 @@  gimple_get_relevant_ref_binfo (tree ref,
 	      || BINFO_N_BASE_BINFOS (binfo) == 0)
 	    return NULL_TREE;
 
-	  base_binfo = BINFO_BASE_BINFO (binfo, 0);
-	  if (BINFO_TYPE (base_binfo) != TREE_TYPE (field))
+	  base_binfo = get_first_base_binfo_with_virtuals (binfo);
+	  if (base_binfo && BINFO_TYPE (base_binfo) != TREE_TYPE (field))
 	    {
 	      tree d_binfo;
 
Index: icln/gcc/testsuite/g++.dg/torture/pr44535.C
===================================================================
--- /dev/null
+++ icln/gcc/testsuite/g++.dg/torture/pr44535.C
@@ -0,0 +1,34 @@ 
+/* { dg-do run } */
+
+namespace FOO {
+
+template <typename T>
+class A
+{
+public:
+    void Enum();
+    virtual void OnProv() = 0;
+    virtual ~A() { }
+};
+typedef A<char> B;
+
+template<typename T>
+void A<T>::Enum ()
+{
+    OnProv ();
+}
+} // namespace FOO
+
+class C {};
+
+class D: public C, public FOO::B {
+public:
+    void OnProv() {}
+};
+
+int main(int argc, char *argv[])
+{
+    D x;
+    x.Enum();
+    return 0;
+}