Patchwork [PR,57670] Do not turn member pointers to builtin_unreachable

login
register
mail settings
Submitter Martin Jambor
Date June 24, 2013, 9:59 p.m.
Message ID <20130624215939.GG31242@virgil.suse>
Download mbox | patch
Permalink /patch/253978/
State New
Headers show

Comments

Martin Jambor - June 24, 2013, 9:59 p.m.
Hi,

I have made a mistake when I decided to redirect member-pointer calls
to builtin_unreachable because I wrongly thought there are two calls,
one for the VMT lookup and other for non-virtual members and thought
only the second got redirected.  However, even though there are two
different lookups, there is only one call and thus we must refrain
from killing it when the value coming from a caller is not an address
of a function.  I have pondered about overloading the polymorphic flag
for marking member pointer calls but eventually decided to introduce a
new one, as there are plenty bits still available in indirect info and
the added complexity of overloading the flag quickly turned ugly.

Bootstrapped and tested on x86_64-linux.  OK for trunk?

Thanks and sorry for the confusion,

Martin


2013-06-21  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/57670
	* cgraph.h (cgraph_indirect_call_info): New flag member_ptr.
	* ipa-prop.c (ipa_print_node_jump_functions): Mark member pointer
	calls in the dump.
	(ipa_note_param_call): Initialize member_ptr flag.
	(ipa_analyze_indirect_call_uses): Set member_ptr flag.
	(ipa_make_edge_direct_to_target): Bail out if member_ptr is set.
	(ipa_write_indirect_edge_info): Stream member_ptr flag.
	(ipa_read_indirect_edge_info): Likewise.

testsuite/
	* g++.dg/ipa/pr57670.C (H): New test.
Jan Hubicka - June 25, 2013, 3:18 a.m.
> Hi,
> 
> I have made a mistake when I decided to redirect member-pointer calls
> to builtin_unreachable because I wrongly thought there are two calls,
> one for the VMT lookup and other for non-virtual members and thought
> only the second got redirected.  However, even though there are two
> different lookups, there is only one call and thus we must refrain
> from killing it when the value coming from a caller is not an address
> of a function.  I have pondered about overloading the polymorphic flag
> for marking member pointer calls but eventually decided to introduce a
> new one, as there are plenty bits still available in indirect info and
> the added complexity of overloading the flag quickly turned ugly.
> 
> Bootstrapped and tested on x86_64-linux.  OK for trunk?
> 
> Thanks and sorry for the confusion,
> 
> Martin
> 
> 
> 2013-06-21  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/57670
> 	* cgraph.h (cgraph_indirect_call_info): New flag member_ptr.
> 	* ipa-prop.c (ipa_print_node_jump_functions): Mark member pointer
> 	calls in the dump.
> 	(ipa_note_param_call): Initialize member_ptr flag.
> 	(ipa_analyze_indirect_call_uses): Set member_ptr flag.
> 	(ipa_make_edge_direct_to_target): Bail out if member_ptr is set.
> 	(ipa_write_indirect_edge_info): Stream member_ptr flag.
> 	(ipa_read_indirect_edge_info): Likewise.
> 
> testsuite/
> 	* g++.dg/ipa/pr57670.C (H): New test.

OK,
thanks!
Honza

Patch

Index: src/gcc/cgraph.h
===================================================================
--- src.orig/gcc/cgraph.h
+++ src/gcc/cgraph.h
@@ -440,6 +440,8 @@  struct GTY(()) cgraph_indirect_call_info
   /* Set when the call is a call of a pointer loaded from contents of an
      aggregate at offset.  */
   unsigned agg_contents : 1;
+  /* Set when this is a call through a member pointer.  */
+  unsigned member_ptr : 1;
   /* When the previous bit is set, this one determines whether the destination
      is loaded from a parameter passed by reference. */
   unsigned by_ref : 1;
Index: src/gcc/ipa-prop.c
===================================================================
--- src.orig/gcc/ipa-prop.c
+++ src/gcc/ipa-prop.c
@@ -288,8 +288,9 @@  ipa_print_node_jump_functions (FILE *f,
 
       ii = cs->indirect_info;
       if (ii->agg_contents)
-	fprintf (f, "    indirect aggregate callsite, calling param %i, "
+	fprintf (f, "    indirect %s callsite, calling param %i, "
 		 "offset " HOST_WIDE_INT_PRINT_DEC ", %s",
+		 ii->member_ptr ? "member ptr" : "aggregate",
 		 ii->param_index, ii->offset,
 		 ii->by_ref ? "by reference" : "by_value");
       else
@@ -1608,6 +1609,7 @@  ipa_note_param_call (struct cgraph_node
   cs->indirect_info->offset = 0;
   cs->indirect_info->polymorphic = 0;
   cs->indirect_info->agg_contents = 0;
+  cs->indirect_info->member_ptr = 0;
   return cs;
 }
 
@@ -1801,6 +1803,7 @@  ipa_analyze_indirect_call_uses (struct c
       struct cgraph_edge *cs = ipa_note_param_call (node, index, call);
       cs->indirect_info->offset = offset;
       cs->indirect_info->agg_contents = 1;
+      cs->indirect_info->member_ptr = 1;
     }
 
   return;
@@ -2212,6 +2215,10 @@  ipa_make_edge_direct_to_target (struct c
       target = canonicalize_constructor_val (target, NULL);
       if (!target || TREE_CODE (target) != FUNCTION_DECL)
 	{
+	  if (ie->indirect_info->member_ptr)
+	    /* Member pointer call that goes through a VMT lookup.  */
+	    return NULL;
+
 	  if (dump_file)
 	    fprintf (dump_file, "ipa-prop: Discovered direct call to non-function"
 				" in %s/%i, making it unreachable.\n",
@@ -3765,6 +3772,7 @@  ipa_write_indirect_edge_info (struct out
   bp = bitpack_create (ob->main_stream);
   bp_pack_value (&bp, ii->polymorphic, 1);
   bp_pack_value (&bp, ii->agg_contents, 1);
+  bp_pack_value (&bp, ii->member_ptr, 1);
   bp_pack_value (&bp, ii->by_ref, 1);
   streamer_write_bitpack (&bp);
 
@@ -3791,6 +3799,7 @@  ipa_read_indirect_edge_info (struct lto_
   bp = streamer_read_bitpack (ib);
   ii->polymorphic = bp_unpack_value (&bp, 1);
   ii->agg_contents = bp_unpack_value (&bp, 1);
+  ii->member_ptr = bp_unpack_value (&bp, 1);
   ii->by_ref = bp_unpack_value (&bp, 1);
   if (ii->polymorphic)
     {
Index: src/gcc/testsuite/g++.dg/ipa/pr57670.C
===================================================================
--- /dev/null
+++ src/gcc/testsuite/g++.dg/ipa/pr57670.C
@@ -0,0 +1,37 @@ 
+/* { dg-do run } */
+/* { dg-options "-O3 -fno-early-inlining" } */
+
+class H
+{
+public:
+  virtual unsigned bar() const { return 16; }
+};
+
+class A : public H
+{
+  unsigned foo(unsigned (H::*func)(void) const) const;
+public:
+  H *h;
+  virtual unsigned bar() const;
+};
+
+unsigned A::foo(unsigned (H::*func)(void) const) const
+{
+  return  (h->*func)();
+}
+
+unsigned A::bar() const
+{
+  return foo(&H::bar);
+}
+
+int main (int argc, char **argv)
+{
+  H h;
+  A a;
+  a.h = &h;
+
+  if (a.bar() != 16)
+    __builtin_abort ();
+  return 0;
+}