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: [PATCH, updated] Vtable pointer verification, C++ front end changes (patch 1 of 3)


Hi, sorry it has taken me so long to get back to this. Hopefully we can wrap it up quickly now that we're back in stage 1.

On 02/25/2013 02:24 PM, Caroline Tice wrote:
-    CXX_FOR_TARGET='$$r/$(HOST_SUBDIR)/gcc/xg++ -B$$r/$(HOST_SUBDIR)/gcc/ -nostdinc++ `if test -f $$r/$(TARGET_SUBDIR)
/libstdc++-v3/scripts/testsuite_flags; then $(SHELL) $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-
includes; else echo -funconfigured-libstdc++-v3 ; fi` -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/li
bstdc++-v3/src/.libs'
+    CXX_FOR_TARGET='$$r/$(HOST_SUBDIR)/gcc/xg++ -B$$r/$(HOST_SUBDIR)/gcc/ -nostdinc++ `if test -f $$r/$(TARGET_SUBDIR)
/libstdc++-v3/scripts/testsuite_flags; then $(SHELL) $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-
includes; else echo -funconfigured-libstdc++-v3 ; fi` -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/li
bstdc++-v3/src/.libs -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/libsupc++/.libs'

You shouldn't need this, since libstdc++ includes libsupc++. And if you did need to do it, it would need to be in configure.ac or it will be discarded by the next autoconf.

+         information aboui which vtable will actually be emitted.  */

"about"

+vtv_finish_verification_constructor_init_function (tree function_body)
+{
+  tree fn;
+
+  finish_compound_stmt (function_body);
+  fn = finish_function (0);
+  DECL_STATIC_CONSTRUCTOR (fn) = 1;
+  decl_init_priority_insert (fn, MAX_RESERVED_INIT_PRIORITY - 1);

Why did you stop using finish_objects? If it was to be able to return the function, you can get that from current_function_decl before calling finish_objects.

Index: gcc/cp/g++spec.c

Changes to g++spec.c only affect the g++ driver, not the gcc driver. Are you sure this is what you want?  Can't you handle this stuff directly in the specs like address sanitizer does?

I haven't seen a response to this comment.

+       vtv_rts.cc \
+       vtv_malloc.cc \
+       vtv_utils.cc

It seems to me that this code belongs in a separate library like libsanitizer, not in libstdc++.

Or this one.

-      switch_to_section (sect);
+      if (sect->named.name
+         && (strcmp (sect->named.name, ".vtable_map_vars") == 0))
+       {
+#if defined (OBJECT_FORMAT_ELF)
+          targetm.asm_out.named_section (sect->named.name,
+                                        sect->named.common.flags
+                                        | SECTION_LINKONCE,
+                                        DECL_NAME (decl));
+          in_section = sect;
+#else
+          switch_to_section (sect);
+#endif
+        }
+      else
+        switch_to_section (sect);


+  if (strcmp (name, ".vtable_map_vars") == 0)
+      flags |= SECTION_LINKONCE;


These changes should not be necessary.  Just set DECL_ONE_ONLY on the vtable
map variables.


I believe this change was necessary so that each vtable map variable
would have its own comdat name and be in its own comdat group...but I
will revisit this and see if we still need it.

What did you find? Perhaps you need to make sure that the map variables are getting passed to comdat_linkage at some point, such as here in vtable_find_or_create_map_decl:

+      DECL_SECTION_NAME (var_decl) = build_string (strlen (sect_name),
+                                                   sect_name);
+      DECL_HAS_IMPLICIT_SECTION_NAME_P (var_decl) = true;
+      DECL_COMDAT_GROUP (var_decl) = get_identifier (var_name);

Here comdat_linkage (var_decl) could replace these three lines and I believe make the above varasm change unnecessary.

+/* This function adds classes we are interested in to a list of
+   classes that is saved during pre-compiled header generation.
...
+/* This function goes through the list of classes we saved before the
+   pre-compiled header generation and calls vtv_save_base_class_info
+   on each one, to build up our class hierarchy data structure.  */

These functions apply to non-PCH compiles as well; I find the mention of PCH here confusing.

+  tree void_ptr_type = build_pointer_type (void_type_node);
+  tree const_char_ptr_type = build_pointer_type
+                                  (build_qualified_type (char_type_node,
+                                                         TYPE_QUAL_CONST));

These are already built, as ptr_type_node and const_string_type_node.

+  arg_types = chainon (arg_types, build_tree_list (NULL_TREE, void_type_node));

And you can use void_list_node instead of building a new void list.

+  arg_types = build_tree_list (NULL_TREE, build_pointer_type (void_ptr_type));
+  arg_types = chainon (arg_types, build_tree_list (NULL_TREE,
+                                                   const_ptr_type_node));
+  arg_types = chainon (arg_types, build_tree_list (NULL_TREE,
+                                                   size_type_node));
+  arg_types = chainon (arg_types, build_tree_list (NULL_TREE, void_type_node));
+
+  init_set_symbol_type = build_function_type (init_set_symbol_type, arg_types);

And build_function_type_list would be more compact.

+   data.  VPTR_ADDRESS is and expression for calculating the correct

"an"

>        /* Check to see if we have found a constructor vtable.  Add its
>           data if appropriate.  */

This comment should still be corrected to say VTT rather than constructor vtable.

+              /* Loop through the initialization values for this vtable to
+                 get all the correct vtable pointer addresses that we need
+                 to add to our set of valid vtable pointers for the current
+                 base class.  */

This still seems to add all the elements of the VTT, not just the ones that could end up assigned to the primary vptr of the class. I guess you don't currently try to distinguish between the different vptrs in an object, and just assume that any vptr can point to any derived vtable? I suppose the only harm there is having a larger than necessary table of acceptable values. But that choice should be mentioned in a comment.

+                  /* We need to check value and find the bit where we
+                     have something with 2 arguments, the first
+                     argument of which is an ADDR_EXPR and the second
+                     argument of which is an INTEGER_CST.  */
+
+                  while (value && TREE_OPERAND_LENGTH (value) == 1
+                         && TREE_CODE (TREE_OPERAND (value, 0)) == ADDR_EXPR)
+                    value = TREE_OPERAND (value, 0);

The comment doesn't match the code; you're testing for something with one operand. And it seems strange to test for "anything with one operand".

+          if (sub_vtt_addr)
+            {
+              already_registered = check_and_record_registered_pairs
+                                                                (vtt_decl,
+                                                                 sub_vtt_addr,
+                                                                 record_type);

This is registering a pointer into the VTT as a valid vtable pointer; it isn't. I think you can discard all the sub_vtt_addr code.

+          && (vtable_decl = get_vtbl_decl_for_binfo (base_binfo))
+          && !(DECL_VTABLE_OR_VTT_P (vtable_decl)
+               && DECL_CONSTRUCTION_VTABLE_P (vtable_decl)))

get_vtbl_decl_for_binfo will never return a construction vtable.

+      if (vtv_debug)
+        str1 = build_string_literal
+                       (IDENTIFIER_LENGTH (DECL_NAME (base_ptr_var_decl)) + 1,
+                        IDENTIFIER_POINTER (DECL_NAME (base_ptr_var_decl)));

Maybe create a build_string_from_id function?

+          struct varpool_node *vp_node = varpool_node_for_decl (vtt_decl);
+          if (vp_node->finalized
...
+              if (vtable_decl)
+                vtable_should_be_output = TREE_ASM_WRITTEN (vtable_decl);

Why are you using finalized for the VTT and TREE_ASM_WRITTEN for normal vtables?

+              if (vtable_decl && vtable_should_be_output
+                  && BINFO_VTABLE (binfo))

I don't think it's possible for a class to have a vtable, but its TYPE_BINFO not have one.

+create_undef_reference_to_vtv_init (tree init_routine_body)

Why do we need this, given that we'll already have references to the various registration functions?

+          tree arg_read_write = build_int_cst (integer_type_node, 1);
+          tree arg_read_only = build_int_cst (integer_type_node, 0);

You can use integer_one_node and integer_zero_node.

+       vtv_finish_verification_constructor_init_function (init_routine_body);
+      allocate_struct_function (current_function_decl, false);

Why the allocate_struct_function? If you're doing something that needs cfun to be set,

  push_cfun (DECL_STRUCT_FUNCTION (current_function_decL));

would be better, but I don't see anything that would need it.

+      tree var_type = build_pointer_type (void_type_node);
+      tree initial_value = build_int_cst (make_node (INTEGER_TYPE), 0);

Again, ptr_type_node and integer_zero_node.

+      /* We cannot mark this variable as read-only otherwise the gold
+         linker will not put it in the relro section. It seems if it
+         is marked as read-only, gold will put it in the .text
+         segment.  */

I'd still like to change this comment. You can't mark the variable as read-only because you want to write to it at runtime.

+vtv_save_base_class_info (tree type)

Having this function (which is called from ...recover_class_info at EOF) right before vtv_save_class_info (which is used after each class definition) is confusing. Maybe change this to vtv_recover_single_class_info?

I'll take another look at the other patches soon.

Jason


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