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
