diff mbox

C++ PATCH for c++/70147 (-fsanitize=vptr, -flifetime-dse, and virtual bases)

Message ID 56EC1EBF.5050408@redhat.com
State New
Headers show

Commit Message

Jason Merrill March 18, 2016, 3:29 p.m. UTC
The first patch factors out testing of current_in_charge_parm from 
various places in the compiler into a new build_if_in_charge function.

The second patch implements Bernd's suggestion for modifying 
-flifetime-dse so that when we have virtual bases, we clobber the entire 
object, but only when we are in charge (and therefore know we are in the 
constructor for a complete object, and don't need to worry about tail 
padding).

The third patch adjusts the -fsanitize=vptr vptr clearing so that we 
don't clear the vptr for a virtual base when we aren't in charge of 
virtual bases, even if the current class shares the vptr from a primary 
virtual base.

The testcase tests all of these changes.

Tested x86_64-pc-linux-gnu, applying to trunk.
diff mbox

Patch

commit 4cffe7ea961ed6b602a954c616f5186f27c85db5
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Mar 17 17:22:43 2016 -0400

    	* class.c (build_if_in_charge): Split out from build_base_path.
    
    	* init.c (expand_virtual_init, expand_default_init): Use it.
    	* call.c (build_special_member_call): Use it.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 34c1d9b..d445163 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8082,11 +8082,7 @@  build_special_member_call (tree instance, tree name, vec<tree, va_gc> **args,
       vtt = decay_conversion (vtt, complain);
       if (vtt == error_mark_node)
 	return error_mark_node;
-      vtt = build3 (COND_EXPR, TREE_TYPE (vtt),
-		    build2 (EQ_EXPR, boolean_type_node,
-			    current_in_charge_parm, integer_zero_node),
-		    current_vtt_parm,
-		    vtt);
+      vtt = build_if_in_charge (vtt, current_vtt_parm);
       if (BINFO_SUBVTT_INDEX (binfo))
 	sub_vtt = fold_build_pointer_plus (vtt, BINFO_SUBVTT_INDEX (binfo));
       else
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index f8ecfa1..866a0a4 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -225,6 +225,24 @@  int n_convert_harshness = 0;
 int n_compute_conversion_costs = 0;
 int n_inner_fields_searched = 0;
 
+/* Return a COND_EXPR that executes TRUE_STMT if this execution of the
+   'structor is in charge of 'structing virtual bases, or FALSE_STMT
+   otherwise.  */
+
+tree
+build_if_in_charge (tree true_stmt, tree false_stmt)
+{
+  gcc_assert (DECL_HAS_IN_CHARGE_PARM_P (current_function_decl));
+  tree cmp = build2 (NE_EXPR, boolean_type_node,
+		     current_in_charge_parm, integer_zero_node);
+  tree type = unlowered_expr_type (true_stmt);
+  if (VOID_TYPE_P (type))
+    type = unlowered_expr_type (false_stmt);
+  tree cond = build3 (COND_EXPR, type,
+		      cmp, true_stmt, false_stmt);
+  return cond;
+}
+
 /* Convert to or from a base subobject.  EXPR is an expression of type
    `A' or `A*', an expression of type `B' or `B*' is returned.  To
    convert A to a base B, CODE is PLUS_EXPR and BINFO is the binfo for
@@ -470,12 +488,9 @@  build_base_path (enum tree_code code,
 	/* Negative fixed_type_p means this is a constructor or destructor;
 	   virtual base layout is fixed in in-charge [cd]tors, but not in
 	   base [cd]tors.  */
-	offset = build3 (COND_EXPR, ptrdiff_type_node,
-			 build2 (EQ_EXPR, boolean_type_node,
-				 current_in_charge_parm, integer_zero_node),
-			 v_offset,
-			 convert_to_integer (ptrdiff_type_node,
-					     BINFO_OFFSET (binfo)));
+	offset = build_if_in_charge
+	  (convert_to_integer (ptrdiff_type_node, BINFO_OFFSET (binfo)),
+	   v_offset);
       else
 	offset = v_offset;
     }
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index a50d92c..497430a 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5642,6 +5642,7 @@  extern tree get_function_version_dispatcher	(tree);
 
 /* in class.c */
 extern tree build_vfield_ref			(tree, tree);
+extern tree build_if_in_charge			(tree true_stmt, tree false_stmt = void_node);
 extern tree build_base_path			(enum tree_code, tree,
 						 tree, int, tsubst_flags_t);
 extern tree convert_to_base			(tree, tree, bool, bool,
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 22c039b..aee3b84 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -1243,12 +1243,7 @@  expand_virtual_init (tree binfo, tree decl)
       /* The actual initializer is the VTT value only in the subobject
 	 constructor.  In maybe_clone_body we'll substitute NULL for
 	 the vtt_parm in the case of the non-subobject constructor.  */
-      vtbl = build3 (COND_EXPR,
-		     TREE_TYPE (vtbl),
-		     build2 (EQ_EXPR, boolean_type_node,
-			     current_in_charge_parm, integer_zero_node),
-		     vtbl2,
-		     vtbl);
+      vtbl = build_if_in_charge (vtbl, vtbl2);
     }
 
   /* Compute the location of the vtpr.  */
@@ -1741,11 +1736,7 @@  expand_default_init (tree binfo, tree true_exp, tree exp, tree init, int flags,
 					&parms, binfo, flags,
 					complain);
       base = fold_build_cleanup_point_expr (void_type_node, base);
-      rval = build3 (COND_EXPR, void_type_node,
-		     build2 (EQ_EXPR, boolean_type_node,
-			     current_in_charge_parm, integer_zero_node),
-		     base,
-		     complete);
+      rval = build_if_in_charge (complete, base);
     }
    else
     {

commit a8ffdc4d5e991bebdaf4e1154147e42ee6d66e40
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Mar 17 17:09:40 2016 -0400

    	Avoid clobbering primary virtual base when not in charge.
    
    	* decl.c (build_clobber_this): Factor out of
    	start_preparsed_function and begin_destructor_body.  Handle
    	virtual bases better.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 497430a..9c2aeb2 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -1991,7 +1991,7 @@  struct GTY(()) lang_type {
 #define CLASSTYPE_VBASECLASSES(NODE) (LANG_TYPE_CLASS_CHECK (NODE)->vbases)
 
 /* The type corresponding to NODE when NODE is used as a base class,
-   i.e., NODE without virtual base classes.  */
+   i.e., NODE without virtual base classes or tail padding.  */
 
 #define CLASSTYPE_AS_BASE(NODE) (LANG_TYPE_CLASS_CHECK (NODE)->as_base)
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 6cc7046..4f29163 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -13716,6 +13716,43 @@  implicit_default_ctor_p (tree fn)
 	  && sufficient_parms_p (FUNCTION_FIRST_USER_PARMTYPE (fn)));
 }
 
+/* Clobber the contents of *this to let the back end know that the object
+   storage is dead when we enter the constructor or leave the destructor.  */
+
+static tree
+build_clobber_this ()
+{
+  /* Clobbering an empty base is pointless, and harmful if its one byte
+     TYPE_SIZE overlays real data.  */
+  if (is_empty_class (current_class_type))
+    return void_node;
+
+  /* If we have virtual bases, clobber the whole object, but only if we're in
+     charge.  If we don't have virtual bases, clobber the as-base type so we
+     don't mess with tail padding.  */
+  bool vbases = CLASSTYPE_VBASECLASSES (current_class_type);
+
+  tree ctype = current_class_type;
+  if (!vbases)
+    ctype = CLASSTYPE_AS_BASE (ctype);
+
+  tree clobber = build_constructor (ctype, NULL);
+  TREE_THIS_VOLATILE (clobber) = true;
+
+  tree thisref = current_class_ref;
+  if (ctype != current_class_type)
+    {
+      thisref = build_nop (build_reference_type (ctype), current_class_ptr);
+      thisref = convert_from_reference (thisref);
+    }
+
+  tree exprstmt = build2 (MODIFY_EXPR, void_type_node, thisref, clobber);
+  if (vbases)
+    exprstmt = build_if_in_charge (exprstmt);
+
+  return exprstmt;
+}
+
 /* Create the FUNCTION_DECL for a function definition.
    DECLSPECS and DECLARATOR are the parts of the declaration;
    they describe the function's name and the type it returns,
@@ -14131,17 +14168,7 @@  start_preparsed_function (tree decl1, tree attrs, int flags)
 	 because part of the initialization might happen before we enter the
 	 constructor, via AGGR_INIT_ZERO_FIRST (c++/68006).  */
       && !implicit_default_ctor_p (decl1))
-    {
-      /* Insert a clobber to let the back end know that the object storage
-	 is dead when we enter the constructor.  */
-      tree btype = CLASSTYPE_AS_BASE (current_class_type);
-      tree clobber = build_constructor (btype, NULL);
-      TREE_THIS_VOLATILE (clobber) = true;
-      tree bref = build_nop (build_reference_type (btype), current_class_ptr);
-      bref = convert_from_reference (bref);
-      tree exprstmt = build2 (MODIFY_EXPR, btype, bref, clobber);
-      finish_expr_stmt (exprstmt);
-    }
+    finish_expr_stmt (build_clobber_this ());
 
   if (!processing_template_decl
       && DECL_CONSTRUCTOR_P (decl1)
@@ -14371,18 +14398,7 @@  begin_destructor_body (void)
       if (flag_lifetime_dse
 	  /* Clobbering an empty base is harmful if it overlays real data.  */
 	  && !is_empty_class (current_class_type))
-	{
-	  /* Insert a cleanup to let the back end know that the object is dead
-	     when we exit the destructor, either normally or via exception.  */
-	  tree btype = CLASSTYPE_AS_BASE (current_class_type);
-	  tree clobber = build_constructor (btype, NULL);
-	  TREE_THIS_VOLATILE (clobber) = true;
-	  tree bref = build_nop (build_reference_type (btype),
-				 current_class_ptr);
-	  bref = convert_from_reference (bref);
-	  tree exprstmt = build2 (MODIFY_EXPR, btype, bref, clobber);
-	  finish_decl_cleanup (NULL_TREE, exprstmt);
-	}
+	finish_decl_cleanup (NULL_TREE, build_clobber_this ());
 
       /* And insert cleanups for our bases and members so that they
 	 will be properly destroyed if we throw.  */

commit 2994636e0a3e0fc05367bd8ba7ae6eaeb2969b93
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Mar 16 17:13:50 2016 -0400

    	PR c++/70147 - handle primary virtual bases
    
    	* class.c (vptr_via_virtual_p): New.
    	(most_primary_binfo): Factor out of build_rtti_vtbl_entries.
    	* cp-ubsan.c (cp_ubsan_dfs_initialize_vtbl_ptrs): Don't clear
    	a vptr from any virtual base in a not-in-charge 'structor.

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 866a0a4..1fcf61e 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -8493,6 +8493,40 @@  get_primary_binfo (tree binfo)
   return copied_binfo (primary_base, binfo);
 }
 
+/* As above, but iterate until we reach the binfo that actually provides the
+   vptr for BINFO.  */
+
+static tree
+most_primary_binfo (tree binfo)
+{
+  tree b = binfo;
+  while (CLASSTYPE_HAS_PRIMARY_BASE_P (BINFO_TYPE (b))
+	 && !BINFO_LOST_PRIMARY_P (b))
+    {
+      tree primary_base = get_primary_binfo (b);
+      gcc_assert (BINFO_PRIMARY_P (primary_base)
+		  && BINFO_INHERITANCE_CHAIN (primary_base) == b);
+      b = primary_base;
+    }
+  return b;
+}
+
+/* Returns true if BINFO gets its vptr from a virtual base of the most derived
+   type.  Note that the virtual inheritance might be above or below BINFO in
+   the hierarchy.  */
+
+bool
+vptr_via_virtual_p (tree binfo)
+{
+  if (TYPE_P (binfo))
+    binfo = TYPE_BINFO (binfo);
+  tree primary = most_primary_binfo (binfo);
+  /* Don't limit binfo_via_virtual, we want to return true when BINFO itself is
+     a morally virtual base.  */
+  tree virt = binfo_via_virtual (primary, NULL_TREE);
+  return virt != NULL_TREE;
+}
+
 /* If INDENTED_P is zero, indent to INDENT. Return nonzero.  */
 
 static int
@@ -9780,17 +9814,7 @@  build_rtti_vtbl_entries (tree binfo, vtbl_init_data* vid)
 
   /* To find the complete object, we will first convert to our most
      primary base, and then add the offset in the vtbl to that value.  */
-  b = binfo;
-  while (CLASSTYPE_HAS_PRIMARY_BASE_P (BINFO_TYPE (b))
-	 && !BINFO_LOST_PRIMARY_P (b))
-    {
-      tree primary_base;
-
-      primary_base = get_primary_binfo (b);
-      gcc_assert (BINFO_PRIMARY_P (primary_base)
-		  && BINFO_INHERITANCE_CHAIN (primary_base) == b);
-      b = primary_base;
-    }
+  b = most_primary_binfo (binfo);
   offset = size_diffop_loc (input_location,
 			BINFO_OFFSET (vid->rtti_binfo), BINFO_OFFSET (b));
 
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 9c2aeb2..999005e 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5681,6 +5681,7 @@  extern void invalidate_class_lookup_cache	(void);
 extern void maybe_note_name_used_in_class	(tree, tree);
 extern void note_name_declared_in_class		(tree, tree);
 extern tree get_vtbl_decl_for_binfo		(tree);
+extern bool vptr_via_virtual_p			(tree);
 extern void debug_class				(tree);
 extern void debug_thunks			(tree);
 extern void set_linkage_according_to_type	(tree, tree);
diff --git a/gcc/cp/cp-ubsan.c b/gcc/cp/cp-ubsan.c
index 75aeeb8..be24a5c 100644
--- a/gcc/cp/cp-ubsan.c
+++ b/gcc/cp/cp-ubsan.c
@@ -283,7 +283,7 @@  cp_ubsan_dfs_initialize_vtbl_ptrs (tree binfo, void *data)
   if (!TYPE_CONTAINS_VPTR_P (BINFO_TYPE (binfo)))
     return dfs_skip_bases;
 
-  if (!BINFO_PRIMARY_P (binfo) || BINFO_VIRTUAL_P (binfo))
+  if (!BINFO_PRIMARY_P (binfo))
     {
       tree base_ptr = TREE_VALUE ((tree) data);
 
@@ -301,11 +301,10 @@  cp_ubsan_dfs_initialize_vtbl_ptrs (tree binfo, void *data)
       tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
       tree stmt = cp_build_modify_expr (vtbl_ptr, NOP_EXPR, vtbl,
 					tf_warning_or_error);
-      if (BINFO_VIRTUAL_P (binfo))
-	stmt = build3 (COND_EXPR, void_type_node,
-		       build2 (NE_EXPR, boolean_type_node,
-			       current_in_charge_parm, integer_zero_node),
-		       stmt, void_node);
+      if (vptr_via_virtual_p (binfo))
+	/* If this vptr comes from a virtual base of the complete object, only
+	   clear it if we're in charge of virtual bases.  */
+	stmt = build_if_in_charge (stmt);
       finish_expr_stmt (stmt);
     }
 
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-11.C b/gcc/testsuite/g++.dg/ubsan/vptr-11.C
new file mode 100644
index 0000000..4516b1e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-11.C
@@ -0,0 +1,84 @@ 
+// PR c++/70147
+// { dg-do run }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+static int ac, ad, bc, bd, cc, cd, dc, dd;
+struct A
+{
+  A ()
+  {
+    ac++;
+  }
+  virtual void f ()
+  {
+  }
+  __attribute__ ((noinline)) ~ A ();
+};
+
+struct D
+{
+  __attribute__ ((noinline)) D (int);
+  ~D ()
+  {
+    dd++;
+  }
+};
+struct B: virtual A, D
+{
+  B ():D (1)
+  {
+    bc++;
+  }
+  virtual void f ()
+  {
+  }
+  ~B ()
+  {
+    bd++;
+  }
+};
+
+struct C: B, virtual A
+{
+  C ()
+  {
+    cc++;
+  }
+  ~C ()
+  {
+    cd++;
+  }
+};
+
+D::D (int x)
+{
+  if (x)
+    throw 1;
+  dc++;
+}
+
+__attribute__ ((noinline, noclone))
+void foo (A * p)
+{
+  p->f ();
+}
+
+A::~A ()
+{
+  foo (this);
+  ad++;
+}
+
+int
+main ()
+{
+  try
+    {
+      C c;
+    }
+  catch ( ...)
+    {
+    }
+  if (ac != 1 || ad != 1 || bc || bd || cc || cd || dc || dd)
+    __builtin_abort ();
+}