This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fwd: [PATCH] Vtable pointer verification, gcc changes (patch 2 of 2)


On 2012-11-16 13:24 , Caroline Tice wrote:

Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 193571)
+++ gcc/cp/decl2.c	(working copy)
@@ -69,8 +69,8 @@ typedef struct priority_info_s {
 static void mark_vtable_entries (tree);
 static bool maybe_emit_vtables (tree);
 static bool acceptable_java_type (tree);
-static tree start_objects (int, int);
-static void finish_objects (int, int, tree);
+static tree start_objects (int, int, const char *);
+static tree finish_objects (int, int, tree);
 static tree start_static_storage_duration_function (unsigned);
 static void finish_static_storage_duration_function (tree);
 static priority_info get_priority_info (int);
@@ -2964,14 +2964,22 @@ generate_tls_wrapper (tree fn)
 }

 /* Start the process of running a particular set of global constructors
-   or destructors.  Subroutine of do_[cd]tors.  */
+   or destructors.  Subroutine of do_[cd]tors.  Also called from
+   vtv_start_verification_constructor_init_function.
+
+   The parameter EXTRA_NAME will be an empty string for most calls.
+   It gets appended to the end of the name of the function being
+   created.  When this function is being called to create a
+   vtable verification constructor init function, EXTRA_NAME will
+   contain a string based on the source file path, to help give the
+   vtable constructor init functions unique names.  */

Why not use the assembler name of the function here? That would be a uniquely mangled name.



+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+/* This file is part of the vtable security implementation.  It collects
+   class hierarchy information about the program being compiled and
+   inserts calls to __VLTRegisterPair, registering this information.  */

Could you include a high-level overview of how vtable security works? A summary based on the design document would be great. Also, a rough flow of how the implementation works.


+
+/* Need to mark this one specially since it needs to be stored in
+ * precompiled header IR */

No '*' at the start of the second line.


+static bool register_all_pairs (tree body);
+static void add_hierarchy_pair (struct vtv_graph_node *,
+                                struct vtv_graph_node *);
+static struct vtv_graph_node *find_graph_node (tree);
+static struct vtv_graph_node *
+                  find_and_remove_next_leaf_node (struct work_node **worklist);
+static void create_undef_reference_to_vtv_init(tree register_pairs_body);
+static bool vtv_register_class_hierarchy_information
+                                                    (tree register_pairs_body);
+
+static void update_class_hierarchy_information (tree, tree);
+struct vtbl_map_node *vtable_find_or_create_map_decl (tree);

Unless you are breaking a mutually recursive cycle, it's better not to declare static functions in advance. Makes it easier to change the function's signature later.


+
+/* As part of vtable verification the compiler generates and inserts calls
+   to __VLTRegisterPair and __VLTChangePermission, which are in libstdc++.

Don't you mean libsupc++ here? I thought that's where the runtime support was going to go into.



+static void
+init_functions (void)
+{
+  tree void_ptr_type = build_pointer_type (void_type_node);
+  tree arg_types = NULL_TREE;
+  tree register_pairs_type = void_type_node;
+  tree change_permission_type = void_type_node;
+#ifdef VTV_DEBUG

Please make this a debug flag. This kind of #ifdef debugging tends to bitrot quickly and it's never available when you need it most (the production compiler has made a bad decision and you need to know what's happening).


+  tree char_ptr_type = build_pointer_type (char_type_node);
+#endif
+
+  if (vlt_change_permission_fndecl != NULL_TREE)
+    return;
+
+  gcc_assert(vlt_register_pairs_fndecl == NULL_TREE);

Space before '('.



+/* This is a helper function for
+   vtv_compute_class_hierarchy_transitive_closure.  It goes through
+   the WORKLIST of class hierarchy nodes looking for a "leaf" node,
+   i.e. a node whose children in the hierarchy have all been
+   processed.  When it finds the next leaf node, it removes it from
+   the linked list (WORKLIST) and returns the node.  */
+
+static struct vtv_graph_node *
+find_and_remove_next_leaf_node (struct work_node **worklist)
+{
+  struct work_node *prev, *cur;
+
+  for (prev = NULL, cur = *worklist; cur; prev = cur, cur = cur->next)

Would this be better implemented as a vec + hash table or pointer set? How large is this worklist?


+    {
+      if (cur->node->num_children == cur->node->num_processed_children)
+        {
+          if (prev == NULL)
+            (*worklist) = cur->next;
+          else
+            prev->next = cur->next;
+
+          cur->next = NULL;
+          return cur->node;
+        }
+    }
+
+  return NULL;
+}
+
+/* In our class hirarchy graph, each class node contains a bitmap,

s/hirarchy/hierarchy/


+   with one bit for each class in the hierarchy.  The bits are set for
+   classes that are descendants in the graph of the current node.
+   Initially the desdendants bitmap is only set for immediate

s/desdendants/descendants/


+      temp_node->descendants = sbitmap_alloc (num_vtable_map_nodes);
+      bitmap_clear (temp_node->descendants);
+      bitmap_set_bit (temp_node->descendants, temp_node->class_uid);
+      for (i = 0; i < temp_node->num_children; ++i)
+        bitmap_ior (temp_node->descendants, temp_node->descendants,
+                        temp_node->children[i]->descendants);
+      for (i = 0; i < temp_node->num_parents; ++i)
+        {
+          temp_node->parents[i]->num_processed_children =
+                    temp_node->parents[i]->num_processed_children + 1;
+          if (!bitmap_bit_p (inserted, temp_node->parents[i]->class_uid))
+            add_to_worklist (&worklist, temp_node->parents[i], inserted);
+        }
+    }
+}
+
+/* Keep track of which pairs we have already created __VLTRegisterPair
+   calls for, to prevent creating duplicate calls within the same
+   compilation unit.  */

Need to document VTABLE_DECL, VPTR_ADDRESS and BASE_CLASS.


+
+static bool
+record_register_pairs (tree vtable_decl, tree vptr_address,
+                       tree base_class)
+{
+  unsigned offset;
+  tree base_id;
+  struct vtbl_map_node *base_vtable_map_node;
+
+  if (TREE_CODE (vptr_address) == ADDR_EXPR
+      && TREE_CODE (TREE_OPERAND (vptr_address, 0)) == MEM_REF)
+    vptr_address = TREE_OPERAND (vptr_address, 0);
+
+  offset = TREE_INT_CST_LOW (TREE_OPERAND (vptr_address, 1));
+
+  if (TREE_CHAIN (base_class))
+    base_id = DECL_ASSEMBLER_NAME (TREE_CHAIN (base_class));
+  else
+    base_id = DECL_ASSEMBLER_NAME (TYPE_NAME (base_class));
+
+  base_vtable_map_node = vtbl_map_get_node (base_id);
+
+  if (vtbl_map_node_registration_find (base_vtable_map_node, vtable_decl,
+                                       offset))
+    return true;
+
+  vtbl_map_node_registration_insert (base_vtable_map_node, vtable_decl,
+				     offset);
+  return false;
+}
+
+/* A class may contain secondary vtables in it, for various reasons.
+     This function goes through the decl chain of a class record
+     looking for any fields that point to secondary vtables, and
+     adding calls to __VLTRegisterPair for the secondary vtable
+     pointers.  */

Need to document BASE_CLASS_DECL_ARG, BASE_CLASS, RECORD_TYPE and BODY.


+
+static void
+register_vptr_fields (tree base_class_decl_arg, tree base_class,
+                      tree record_type, tree body)
+{
+  tree vtbl_var_decl;
+  tree arg1;
+  tree arg2;
+  int hint = 0;
+
+  if (TREE_CODE (record_type) != RECORD_TYPE)
+    return;
+
+  vtbl_var_decl = get_vtbl_decl_for_binfo (TYPE_BINFO (record_type));
+
+  if (vtbl_var_decl)
+    {
+      tree ztt_decl = DECL_CHAIN (vtbl_var_decl);
+      bool already_registered = false;
+
+      /* construction vtable */

What does this comment mean? Please make comments full sentences that clarify what is happening in the code.


+      if (ztt_decl != NULL_TREE
+          && (DECL_NAME (ztt_decl))
+          && (strncmp (IDENTIFIER_POINTER (DECL_NAME (ztt_decl)),
+                       "_ZTT", 4) == 0))
+        {
+          tree values = DECL_INITIAL (ztt_decl);
+          struct varpool_node * vp_node = varpool_node_for_decl (ztt_decl);
+          if (vp_node->finalized
+              && TREE_ASM_WRITTEN (ztt_decl)
+              && (values != NULL_TREE)
+              && (TREE_CODE (values) == CONSTRUCTOR)
+              && (TREE_CODE (TREE_TYPE (values)) == ARRAY_TYPE))
+            {
+              tree call_expr = NULL_TREE;
+              unsigned HOST_WIDE_INT cnt;
+              constructor_elt *ce;
+
+              for (cnt = 0;
+                   VEC_iterate (constructor_elt, CONSTRUCTOR_ELTS (values),
+				cnt, ce);
+                   cnt++)
+                {
+                  tree value = ce->value;
+                  tree val_vtbl_decl = TREE_OPERAND (TREE_OPERAND (value, 0),
+                                                     0);
+                  int len1;
+                  int len2;
+
+                  if (TREE_CODE (val_vtbl_decl) == ADDR_EXPR
+                      && TREE_CODE (TREE_OPERAND (val_vtbl_decl, 0))
+                                                                   == VAR_DECL)
+                    val_vtbl_decl = TREE_OPERAND (val_vtbl_decl, 0);
+
+                  gcc_assert (TREE_CODE (val_vtbl_decl) == VAR_DECL);
+
+                  len1 = strlen (IDENTIFIER_POINTER
+				     (DECL_NAME
+				          (TREE_OPERAND
+					     (base_class_decl_arg, 0))));
+                  len2 = strlen (IDENTIFIER_POINTER
+				     (DECL_NAME (val_vtbl_decl)));
+                  arg1 = build_string_literal (len1,
+                                               IDENTIFIER_POINTER
+					        (DECL_NAME
+						 (TREE_OPERAND
+						  (base_class_decl_arg, 0))));
+                  arg2 = build_string_literal (len2,
+                                               IDENTIFIER_POINTER
+					        (DECL_NAME (val_vtbl_decl)));
+
+                  already_registered = record_register_pairs (val_vtbl_decl,
+                                                              value,
+                                                              base_class);
+
+                  if (already_registered)
+                    continue;
+

Can you add some commentary about what's going on in this loop? Seems pretty obscure.


+#ifdef VTV_DEBUG
+                  call_expr = build_call_expr
+		                        (vlt_register_pairs_fndecl, 7,
+					 base_class_decl_arg, value,
+                                         build_int_cst (integer_type_node,
+                                                        hint),
+					 arg1,
+					 build_int_cst (integer_type_node,
+							len1),
+					 arg2,
+					 build_int_cst (integer_type_node,
+							len2));
+#else
+                  call_expr = build_call_expr
+                                        (vlt_register_pairs_fndecl, 3,
+                                         base_class_decl_arg, value,
+                                         build_int_cst (integer_type_node,
+                                                        hint));
+#endif
+		  append_to_statement_list (call_expr, &body);
+                }
+            }
+        }
+    }
+}
+
+/* This function iterates through all the vtable it can find from the

s/vtable/vtables/


+   BINFO of a class, to make sure we have found ALL of the vtables
+   that an object of that class could point to.  Generate calls to
+   __VLTRegisterPair for those vtable pointers that we find.  */
+
+static void
+register_other_binfo_vtables (tree binfo, tree body, tree arg1, tree str1,
+                              int len1, tree str2, int len2, tree base_class)

All these parameters need documentation.



+/* The set of valid vtable pointers for any given class are stored in
+   a hash table.  For reasons of efficiency, that hash table size is
+   always a power of two.  In order to try to prevent re-sizing the
+   hash tables very often, we pass __VLTRegisterPair an initial guess
+   as to the number of entries the hashtable will eventually need
+   (rounded up to the nearest power of two).  This function take the

s/take/takes/


+   class information we have collected for a particular class and
+   calculates the hash table size guess.  */
+
+static int
+guess_num_vtable_pointers (struct vtv_graph_node *class_node)

CLASS_NODE needs to be documented.



+/* This function goes through our internal class hierarchy & vtable
+   pointer data structure and outputs calls to __VLTRegisterPair for
+   every class-vptr pair (for those classes whose vtable would be
+   output in the current compilation unit).  These calls get put into
+   our constructor initialization function.  */
+
+static bool
+register_all_pairs (tree body)
+{

BODY needs to be documented.



+      /* If this function is going into the preinit_array, then we
+         need to manually call __VLTChangePermission, rather than
+         depending on initialization prioritys in vtv_init. */
+      if (flag_vtable_verify == VTV_PREINIT_PRIORITY)
+        {
+          /* Pass __VLTP_READ_WRITE value as defined in vtv_rts.h */
+          tree arg_read_write = build_int_cst (integer_type_node, 1);
+          tree arg_read_only = build_int_cst (integer_type_node, 0);
+
+          tree call_rw_expr = build_call_expr (vlt_change_permission_fndecl,
+                                               1, arg_read_write);
+          tree_stmt_iterator i = tsi_start(register_pairs_body);
+          /* Insert at the beginning of the register pairs routine */
+          tsi_link_before(&i, call_rw_expr, TSI_SAME_STMT);

Space before '(' and blank line before comment.


+
+          tree call_r_expr = build_call_expr (vlt_change_permission_fndecl,
+                                              1, arg_read_only);
+          append_to_statement_list (call_r_expr, &register_pairs_body);
+        }
+
+      if (flag_vtable_verify == VTV_STANDARD_PRIORITY)
+        create_undef_reference_to_vtv_init(register_pairs_body);

Space before '('.


+  }
+
+  return registered_something;
+}
+
+
+/* Generate the special constructor function that calls
+   __VLTChangePermission and __VLTRegisterPairs, and give it a very
+   high initialization priority.  */
+
+void
+vtv_generate_init_routine(const char * filename)

Space before '('. No space after '*' (happens in other pointer declarations as well). FILENAME needs documenting.



+  char *var_name = NULL;
+  struct vtbl_map_node *vtable_map_node = NULL;
+
+
+  /* Verify the type has an associated vtable */

End comments with '. */' (my kingdom for a gcc-specific linter!).



Property changes on: gcc/cp/vtable-class-hierarchy.c
___________________________________________________________________
Added: svn:eol-style
   + LF

Please remove this svn attribute.




 /* in dump.c */
 extern bool cp_dump_tree			(void *, tree);
Index: gcc/timevar.def
===================================================================
--- gcc/timevar.def	(revision 193571)
+++ gcc/timevar.def	(working copy)
@@ -257,6 +257,7 @@ DEFTIMEVAR (TV_TREE_UNINIT           , "
 DEFTIMEVAR (TV_PLUGIN_INIT           , "plugin initialization")
 DEFTIMEVAR (TV_PLUGIN_RUN            , "plugin execution")
 DEFTIMEVAR (TV_GIMPLE_SLSR           , "straight-line strength reduction")
+DEFTIMEVAR (TV_VTABLE_VERIFICATION   , "tree vtable verification")

Just "vtable verification".



 #endif /* ! GCC_FLAG_TYPES_H */
Index: gcc/tree-vtable-verify.c
===================================================================
--- gcc/tree-vtable-verify.c	(revision 0)
+++ gcc/tree-vtable-verify.c	(revision 0)
@@ -0,0 +1,1053 @@
+/*   Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
+    Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+/* Virtual Table Pointer Security Pass - Detect corruption of vtable pointers
+   before using them for virtual method dispatches.  */

I'd like to see an overview of the actual checking pass here. The highlights and how it's implemented in the file.


+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
+#include "tree.h"
+#include "cp/cp-tree.h"
+#include "tm_p.h"
+#include "basic-block.h"
+#include "output.h"
+#include "tree-flow.h"
+#include "tree-dump.h"
+#include "tree-pass.h"
+#include "timevar.h"
+#include "cfgloop.h"
+#include "flags.h"
+#include "tree-inline.h"
+#include "tree-scalar-evolution.h"
+#include "diagnostic-core.h"
+#include "gimple-pretty-print.h"
+#include "toplev.h"
+#include "langhooks.h"
+
+#include "tree-vtable-verify.h"
+
+unsigned num_vtable_map_nodes = 0;
+
+static GTY(()) tree verify_vtbl_ptr_fndecl = NULL_TREE;
+
+unsigned int vtable_verify_main (void);
+static bool gate_tree_vtable_verify (void);
+static void build_vtable_verify_fndecl (void);

No forward declaration of static functions, please.


+
+/* The following few functions are for the vtbl pointer hash table
+   in the 'registered' field of the struct vtable_map_node.  The hash
+   table keeps track of which vtable pointers have been used in
+   calls to __VLTRegisterPair with that particular vtable map variable.  */
+
+/* This function checks to see if a particular VTABLE_DECL and OFFSET are
+   already in the 'registered' hash table for NODE.  */
+
+bool
+vtbl_map_node_registration_find (struct vtbl_map_node *node,
+                                 tree vtable_decl,
+                                 unsigned offset)
+{
+  struct vtable_registration key;
+  struct vtable_registration **slot;
+
+  gcc_assert (node);
+  gcc_assert (node->registered);

gcc_assert (node && node->registered);



+
+/* This function inserts VTABLE_DECL and OFFSET into the 'registered'
+   hash table for NODE.  */
+
+void
+vtbl_map_node_registration_insert (struct vtbl_map_node *node,
+                                   tree vtable_decl,
+                                   unsigned offset)
+{
+  struct vtable_registration key;
+  struct vtable_registration **slot;
+
+  if (!node || !(node->registered))

No need to surround node->registered in ().


+    return;
+
+  key.vtable_decl = vtable_decl;
+  slot = (struct vtable_registration **) htab_find_slot (node->registered,
+                                                         &key, INSERT);
+
+  if (!(*slot))

Likewise here.


+    {
+      unsigned i;
+      struct vtable_registration *node;
+      node = (struct vtable_registration *)
+                                 xmalloc (sizeof (struct vtable_registration));
+      node->vtable_decl = vtable_decl;
+      node->offsets = (unsigned *) xmalloc (10 * sizeof (unsigned));
+      for (i= 0; i < 10; ++i)
+        node->offsets[i] = 0;

Why 10? Why not 5 or 20? Can you put a comment here about it?



+      node->offsets[0] = offset;
+      node->cur_offset = 1;
+      node->max_offsets = 10;
+      *slot = node;
+    }
+  else
+    {
+      /* We found the vtable_decl slot; we need to see if it already
+         contains the offset.  If not, we need to add the offset.  */
+      unsigned i;
+      bool found = false;
+      for (i = 0; (i < (*slot)->cur_offset) && !found; ++i)

No need to surround the first part of the && in ().


+        if ((*slot)->offsets[i] == offset)
+          found = true;
+
+      if (!found)
+        {
+	  /* Re-size the offset array if necessary.*/

'. */'


+          if ((*slot)->cur_offset == (*slot)->max_offsets)
+            {
+              unsigned new_max = 2 * (*slot)->max_offsets;
+              (*slot)->offsets = (unsigned *)
+                  xrealloc ((*slot)->offsets, new_max * sizeof (unsigned));
+
+              for (i = (*slot)->max_offsets; i < new_max; ++i)
+                (*slot)->offsets[i] = 0;
+              (*slot)->max_offsets = new_max;
+            }
+	  /* Insert the new offset. */

Likewise.


+          (*slot)->offsets[(*slot)->cur_offset] = offset;
+          (*slot)->cur_offset = (*slot)->cur_offset + 1;
+        }
+    }
+}
+
+/* Hashtable functions for vtable_registration hashtables.  */
+
+static hashval_t
+hash_vtable_registration (const void * p)
+{
+  const struct vtable_registration *n = (const struct vtable_registration *) p;
+  return (hashval_t) (DECL_UID (n->vtable_decl));
+}
+
+static int
+eq_vtable_registration (const void *p1, const void *p2)
+{
+  const struct vtable_registration *n1 =
+                                    (const struct vtable_registration *) p1;
+  const struct vtable_registration *n2 =
+                                    (const struct vtable_registration *) p2;
+  return (DECL_UID (n1->vtable_decl) == DECL_UID (n2->vtable_decl));
+}
+
+/* End of hashtable functions for "registered" hashtables*/

'. */'


+
+
+/* Here are the three data structures into which we insert vtable map nodes.
+   We use three data structures because of the vastly different ways we need
+   to find the nodes for various tasks (see comments in tree-vtable-verify.h
+   for more details.  */
+
+/* Vtable map variable nodes stored in a hash table.  */
+static htab_t vtbl_map_hash = NULL;
+
+/* Vtable map variable nodes stored in a linked list.  */
+struct vtbl_map_node *vtbl_map_nodes = NULL;
+
+/* Vtable map variable nodes stored in an array.  */
+struct vtbl_map_node **vtbl_map_nodes_array = NULL;

A vec<> might be easier here.


+
+/* This function inserts a vtable map variable NODE into the array.  */
+
+static void
+vtable_map_array_insert (struct vtbl_map_node *node)
+{
+  static unsigned array_size = 0;
+  unsigned i;
+
+  if (vtbl_map_nodes_array == NULL
+      || array_size == 0)
+    {
+      array_size = 16;
+      vtbl_map_nodes_array = (struct vtbl_map_node **)
+                       xmalloc (array_size * sizeof (struct vtbl_map_node *));
+      memset (vtbl_map_nodes_array, 0,
+              array_size * sizeof (struct vtbl_map_node *));

Definitely. Please make vtbl_map_nodes_array a vec<>.



+/* Return vtbl_map node for CLASS_NAME  without creating a new one.  */
+struct vtbl_map_node *
+vtbl_map_get_node (const_tree class_name)
+{
+  struct vtbl_map_node key;
+  struct vtbl_map_node **slot;
+
+  if (!vtbl_map_hash)
+    return NULL;
+
+  key.class_name = CONST_CAST2 (tree, const_tree, class_name);
+  slot = (struct vtbl_map_node **) htab_find_slot (vtbl_map_hash, &key,
+                                                   NO_INSERT);
+  if (!slot)
+    return NULL;
+  return *slot;
+}
+
+/* Return vtbl_map node assigned to BASE_CLASS_TYPE.  Create new one
+   when needed.  */
+struct vtbl_map_node *

Blank line after comment.


+find_or_create_vtbl_map_node (tree base_class_type)
+{
+  struct vtbl_map_node key;
+  struct vtbl_map_node *node;
+  struct vtbl_map_node **slot;
+  unsigned i;
+
+  if (!vtbl_map_hash)
+    vtbl_map_hash = htab_create (10, hash_vtbl_map_node,
+                                 eq_vtbl_map_node, NULL);
+
+  if (TREE_CHAIN (base_class_type))
+    key.class_name = DECL_ASSEMBLER_NAME (TREE_CHAIN (base_class_type));
+  else
+    key.class_name = DECL_ASSEMBLER_NAME (TYPE_NAME (base_class_type));
+  slot = (struct vtbl_map_node **) htab_find_slot (vtbl_map_hash, &key,
+                                                   INSERT);
+  if (*slot)
+    return *slot;
+
+  node = (struct vtbl_map_node *) xmalloc (sizeof (struct vtbl_map_node));
+  node->vtbl_map_decl = NULL_TREE;
+  node->class_name = key.class_name;
+  node->uid = num_vtable_map_nodes++;
+
+  node->class_info = (struct vtv_graph_node *)
+                                      xmalloc (sizeof (struct vtv_graph_node));

s/xmalloc/XNEW/


+  node->class_info->class_type = base_class_type;
+  node->class_info->class_uid = node->uid;
+  node->class_info->max_parents = 4;
+  node->class_info->max_children = 4;
+  node->class_info->num_parents = 0;
+  node->class_info->num_children = 0;
+  node->class_info->num_processed_children = 0;
+  node->class_info->parents = (struct vtv_graph_node **)
+                                xmalloc (4 * sizeof (struct vtv_graph_node *));
+  node->class_info->children = (struct vtv_graph_node **)
+                                xmalloc (4 * sizeof (struct vtv_graph_node *));

Likewise here.


+ for (i = 0; i < 4; ++i)

Another hard-coded constant. Maybe a comment of why 4 is the magic number?


+    {
+      node->class_info->parents[i] = NULL;
+      node->class_info->children[i] = NULL;
+    }
+
+  node->registered = htab_create (16, hash_vtable_registration,
+                                  eq_vtable_registration, NULL);
+  node->is_used = false;
+  node->next = vtbl_map_nodes;
+  if (vtbl_map_nodes)
+    vtbl_map_nodes->prev = node;
+
+  vtable_map_array_insert (node);
+
+  vtbl_map_nodes = node;
+  *slot = node;
+  return node;
+}
+
+/* This function take a tree type, NODE, and checks to see if the name
+   of the type is "__vtbl_ptr_type".  */
+
+static int
+type_name_is_vtable_pointer (tree node)

Make it return bool please.



+/* Given a gimple STMT, this function checks to see if the statement
+   is an assignment, the rhs of which is getting the vtable pointer
+   value out of an object.  (i.e. it's the value we need to verify
+   because its the vtable pointer that will be used for a virtual
+   call).  */
+
+static int
+is_vtable_assignment_stmt (gimple stmt)

Likewise.


+{
+
+  if (gimple_code (stmt) != GIMPLE_ASSIGN)
+    return 0;
+  else
+    {
+      tree lhs = gimple_assign_lhs (stmt);
+      tree rhs = gimple_assign_rhs1 (stmt);
+
+      if (TREE_CODE (lhs) != SSA_NAME)
+        return 0;
+
+      if (TREE_CODE (TREE_TYPE (lhs)) != POINTER_TYPE)

POINTER_TYPE_P (TREE_TYPE (lhs))


+        return 0;
+
+      if (TREE_CODE (TREE_TYPE (TREE_TYPE (lhs))) != POINTER_TYPE)

Likewise.


+        return 0;
+
+      if (! type_name_is_vtable_pointer (TREE_TYPE (TREE_TYPE (lhs))))

Why not fold the test above inside type_name_is_vtable_pointer?


+}
+
+/* This function, given the BINFO for a virtual type, gets the vtable decl

Change to "Give the BINFO for a virtual type, this function..."


+   for that BINFO.  */
+
+static tree
+my_get_vtbl_decl_for_binfo (tree binfo)
+{
+  tree decl;
+
+  decl = BINFO_VTABLE (binfo);
+  if (decl && TREE_CODE (decl) == POINTER_PLUS_EXPR)
+  {
+    gcc_assert (TREE_CODE (TREE_OPERAND (decl, 0)) == ADDR_EXPR);
+    decl = TREE_OPERAND (TREE_OPERAND (decl, 0), 0);
+  }
+  if (decl)
+    gcc_assert (TREE_CODE (decl) == VAR_DECL);
+
+  return decl;
+}
+
+/* STMT is a gimple statment that uses OLD_VAR.  This function finds the
+   use of OLD_VAR in STMT and replaces it with NEW_VAR.  STMT is either
+   an assignment statement or a call statement.  */
+
+static bool
+find_and_replace_var (gimple stmt, tree old_var, tree new_var)
+{
+  bool found = false;
+
+  if (!stmt || ! old_var || !new_var)
+    return found;
+
+  if ((TREE_CODE (old_var) != SSA_NAME)
+      || (TREE_CODE (new_var) != SSA_NAME))
+    return found;
+
+  if (gimple_code (stmt) == GIMPLE_ASSIGN)
+    {

This looks too convoluted. Why not just traverse all the gimple operands and replace OLD_VAR with NEW_VAR?


You can either use the visitor pattern with walk_gimple_op() or knowing that you only want to walk GIMPLE_ASSIGNs, simply loop over the operands with for (i = 0; i < gimple_num_ops (stmt); i++).

You also want to handle GIMPLE_CALLs, so walk_gimple_op() may be the easiest way.


+/* Search through all the statements in a basic block, searching for
+   virtual method calls.  For each virtual method dispatch, find the
+   vptr value used, and the statically declared type of the object;
+   retrieve the vtable map variable for the type of the object;
+   generate a call to __VLTVerifyVtablePointer; and insert the
+   generated call into the basic block, after the point where the vptr
+   value is gotten out of the object and before the virtual method
+   dispatch. Make the virtual method dispatch depend on the return
+   value from the verification call, so that subsequent optimizations
+   cannot reorder the two calls.  */
+
+static void
+verify_bb_vtables (basic_block bb)
+{
+  gimple_seq stmts;
+  gimple stmt = NULL;
+  gimple_stmt_iterator gsi_vtbl_assign;
+  gimple_stmt_iterator gsi_virtual_call;
+  tree this_object;
+
+  /* Search the basic block to see if it contains a virtual method
+     call, i.e. a call with the tree code OBJ_TYPE_REF  */

Please describe a bit more how you go about finding this. The description seems to imply a simple pattern matching, but the code in the loop body is doing a whole bunch of seemingly unrelated checks.


+
+  stmts = bb_seq (bb);
+  gsi_virtual_call = gsi_start (stmts);
+  this_object = NULL_TREE;
+  for (; !gsi_end_p (gsi_virtual_call); gsi_next (&gsi_virtual_call))
+    {
+      stmt = gsi_stmt (gsi_virtual_call);
+      if (gimple_code (stmt) == GIMPLE_CALL)
+        {
+          tree fncall = gimple_call_fn (stmt);
+          if (TREE_CODE (fncall) == OBJ_TYPE_REF)
+            {
+              bool found = false;
+              tree vtable_offset_var = NULL_TREE;
+              gimple def_stmt;
+              gimple prev_use = NULL;
+
+              /* The first argument to the function must be "this", a pointer
+                 to the object itself.  */
+
+              this_object = gimple_call_arg (stmt, 0);
+
+              /* Get the SSA variable that contains the dereferenced _vptr
+                 field + table start offset.  */
+
+              if (TREE_OPERAND (fncall, 0)
+                  && TREE_CODE (TREE_OPERAND (fncall, 0)) == SSA_NAME)
+                {
+                  tree rhs = NULL_TREE;
+                  gimple phi_stmt = NULL;
+                  gimple phi_prev_use = NULL;
+                  bool  found_phi = false;
+
+                  vtable_offset_var = TREE_OPERAND (fncall, 0);
+
+                  prev_use = stmt;
+                  def_stmt = SSA_NAME_DEF_STMT (vtable_offset_var);
+
+		  /* Search backwards through the def_stmt chain, to try
+		     to find the assignment statement where the rhs of
+		     the assignment contains the "._vptr" field (the vtable
+		     pointer). */
+
+		  /*
+                  while (gimple_code (def_stmt) == GIMPLE_PHI)
+                    {
+                      if (!found_phi)
+                        {
+                          phi_prev_use = prev_use;
+                          phi_stmt = def_stmt;
+                          found_phi = true;
+                        }
+                      def_stmt = get_phi_def_stmt (def_stmt);
+                    }
+		  */

What is this?


+
+                  gcc_assert (def_stmt != NULL);
+                  gcc_assert (gimple_code (def_stmt) == GIMPLE_ASSIGN);
+
+                  if (gimple_assign_lhs (def_stmt)
+                      && TREE_CODE (gimple_assign_lhs (def_stmt)) == SSA_NAME
+                      && get_gimple_rhs_class (gimple_expr_code (def_stmt))
+                                                          == GIMPLE_SINGLE_RHS)
+                    rhs = gimple_assign_rhs1 (def_stmt);
+
+                  if (rhs
+                      && TREE_CODE (rhs) == MEM_REF
+                      && TREE_CODE (TREE_OPERAND (rhs, 0)) == SSA_NAME)
+                    {
+                      prev_use = def_stmt;
+                      def_stmt = SSA_NAME_DEF_STMT (TREE_OPERAND (rhs, 0));
+                    }
+
+		  /*
+                  if (gimple_code (def_stmt) == GIMPLE_PHI)
+                    {
+                      if (!found_phi)
+                        {
+                          phi_prev_use = prev_use;
+                          phi_stmt = def_stmt;
+                          found_phi = true;
+                        }
+                      def_stmt = get_phi_def_stmt (def_stmt);
+                    }
+		  */

What is this?


+                  while (def_stmt
+                         && !is_vtable_assignment_stmt (def_stmt))
+                    {
+                      tree lhs = gimple_assign_lhs (def_stmt);
+                      if (!lhs
+                          || !TREE_CODE (lhs) == SSA_NAME)

Only indent predicates if they don't fit on the line.


+                        {
+                          def_stmt = NULL;
+                          break;
+                        }
+                      if (! def_stmt)
+                        break;
+
+                      if (gimple_assign_rhs1 (def_stmt)
+                          && TREE_CODE (gimple_assign_rhs1 (def_stmt))
+                                                                   == SSA_NAME)
+                        {
+                          prev_use = def_stmt;
+                          rhs = gimple_assign_rhs1 (def_stmt);
+                          def_stmt = SSA_NAME_DEF_STMT (rhs);
+                        }
+                      else if ((get_gimple_rhs_class
+                                  (gimple_expr_code (def_stmt))
+			                                  != GIMPLE_SINGLE_RHS)
+                               && gimple_assign_rhs2 (def_stmt)
+                               && TREE_CODE (gimple_assign_rhs2 (def_stmt))
+                                                                   == SSA_NAME)
+                        {
+                          prev_use = def_stmt;
+                          rhs = gimple_assign_rhs2 (def_stmt);
+                          def_stmt = SSA_NAME_DEF_STMT (rhs);
+                        }
+                      else
+                        def_stmt = NULL;
+
+                      if (!def_stmt)
+                        break;
+
+		      /*
+                      if (def_stmt != NULL
+                          && gimple_code (def_stmt) == GIMPLE_PHI)
+                        {
+                          if (!found_phi)
+                            {
+                              phi_prev_use = prev_use;
+                              phi_stmt = def_stmt;
+                              found_phi = true;
+                            }
+                          def_stmt = get_phi_def_stmt (def_stmt);
+                        }
+		      */
+                    }

Please factor out this logic into one or more function helpers.



+                      if (found)
+                        {
+                          tree object_rhs = TREE_TYPE (this_object);
+                          tree lhs;
+                          tree vtbl_var_decl = NULL_TREE;
+                          tree vtbl = NULL_TREE;
+                          tree var_id;
+                          gimple_seq pre_p = NULL;
+                          struct vtbl_map_node *vtable_map_node = NULL;
+                          tree vtbl_decl = NULL_TREE;

All this should be factored into a helper function. By the amount of code you have here, I would say that you will want more than one. This is the main logic of the transformation. It should be very readable. Right now, all the logic is intermixed with low-level details, it's hard to follow.



+                          else
+                            internal_error ("if (verify_vtbl_ptr_fndecl && vtbl_var_decl) FAILED.");

gcc_unreachable () or make the message mean something more.


+                        }
+                      else
+                        internal_error ("if (found) FAILED.");

Likewise.


+                    }
+                }
+              else
+                internal_error ("if fncall...FAILED.");

Likewise.



Property changes on: gcc/tree-vtable-verify.c
___________________________________________________________________
Added: svn:eol-style
   + LF

Remove this attribute.



Property changes on: gcc/tree-vtable-verify.h
___________________________________________________________________
Added: svn:eol-style
   + LF

Remove this attribute.




+void +assemble_vtv_preinit_initializer (tree fn_decl) +{

Missing documentation.



The pass should also be documented in doc/invoke.texi.


I can continue reviewing the changes to the middle end, but you'll need a C++ reviewer to go over the changes to the C++ FE. I have only made cosmetic comments in the C++ bits. Please split the patch in two, to simplify reviewing (i.e., separate the C++ from the ME parts)


Thanks. Diego.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]