I'm also touching on the middle-end and library changes, but would
appreciate a more thorough review from others in those areas.
On 01/23/2013 05:25 PM, Caroline Tice wrote:
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?
@@ -17954,6 +17954,10 @@ mark_class_instantiated (tree t, int ext
if (! extern_p)
{
CLASSTYPE_DEBUG_REQUESTED (t) = 1;
+
+ if (flag_vtable_verify)
+ vtv_save_class_info (t);
Why do you need this here as well as in finish_struct_1?
-static tree
+tree
get_mangled_id (tree decl)
It doesn't look like you call get_mangled_id anywhere, so I think you
don't need this change anymore.
finalize_compilation_unit ();
+ if (flag_vtable_verify)
+ {
+ /* Generate the special constructor initialization function that
+ calls __VLTRegisterPairs, and give it a very high initialization
+ priority. */
+ vtv_generate_init_routine ();
+ }
Why do you want to do this after cgraph finalization? Is it so that you
know which vtables are really being emitted? Please explain in the comment.
+ if (flag_vtable_verify
+ && strstr (IDENTIFIER_POINTER (DECL_NAME (fn)), ".vtable"))
+ return fn;
How would this function end up with .vtable at the end of its name?
+ if (TREE_CHAIN (base_class))
+ class_type_decl = TREE_CHAIN (base_class);
+ else
+ class_type_decl = TYPE_NAME (base_class);
+
+ /* Temporarily remove any qualifiers on type. */
+ type_decl_type = TREE_TYPE (class_type_decl);
+ save_quals = TYPE_QUALS (type_decl_type);
+ reset_type_qualifiers (null_quals, type_decl_type);
+
+ base_id = DECL_ASSEMBLER_NAME (TREE_CHAIN (base_class));
I think you want TYPE_LINKAGE_IDENTIFIER here.
I don't understand what the qualifier business is trying to accomplish,
especially since you never use type_decl_type. You do this in several
places, but it should never be necessary; classes don't have any
qualifiers.
+ 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);
Here you're doing two hash table lookups when one would be enough.
+ vtbl_var_decl = get_vtbl_decl_for_binfo (TYPE_BINFO (record_type));
Here you could use CLASSTYPE_VTABLES (record_type).
+ /* Check to see if we have found a constructor vtable. Add its
+ data if appropriate. */
_ZTT indicates a VTT (vtable table,
http://mentorembedded.github.com/cxx-abi/abi.html#vtable-ctor), which
points to construction vtables, but is not itself a construction vtable.
In register_vptr_fields it seems that given a base B and derived D, you
are adding all the elements of D's VTT to the list of valid vtables for
a B, but this isn't correct; elements of D's VTT may be vtables for
other bases, or other VTTs. I think this might even miss some
potential B vtables by not walking into sub-VTTs.
Since this function is just trying to register construction vtables, I
think calling it "register_construction_vtables" would be clearer.
+ && (strncmp (IDENTIFIER_POINTER (DECL_NAME (ztt_decl)),
+ "_ZTT", 4) == 0))
I think relying on the mangled name like this is going to be fragile;
for instance, some targets use two underscores at the beginning of the
mangled name. Probably better to assume that if CLASSTYPE_VBASECLASSES
(record_type), it's the VTT; that's what build_special_member_call does.
Or better yet, encapsulate that logic in a get_vtt function.
+ && TREE_CODE (TREE_OPERAND (val_vtbl_decl, 0))
+ == VAR_DECL)
Odd indentation.
+ 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 + 1,
+ IDENTIFIER_POINTER
+ (DECL_NAME
+ (TREE_OPERAND
+ (base_class_decl_arg, 0))));
+ arg2 = build_string_literal (len2 + 1,
+ IDENTIFIER_POINTER
+ (DECL_NAME (val_vtbl_decl)));
Since these are only used when debugging, let's only initialize them
when debugging, too. And instead of strlen (IDENTIFIER_POINTER, use
IDENTIFIER_LENGTH.
2). A linked list of all
+ vtbl_map_nodes ("vtbl_map_nodes") for easy iteration through all of
+ them; and 3). An array of vtbl_map_nodes, where the array index
+ corresponds to the unique id of the vtbl_map_node, which gives us
+ an easy way to use bitmaps to represent and find the vtable map
+ nodes.
What's hard about iteration over an array? FOR_EACH_VEC_ELT is pretty
simple to use. Is there another reason the list is desirable?
For that matter, you don't need the array, either; you can just use
TYPE_UID for a bitmap key and use htab_traverse to iterate over all
elements.
+register_other_binfo_vtables (tree binfo, tree body, tree arg1, tree str1,
It seems like this function is saying that it's OK for the BINFO vptr to
point to the vtable for one of its bases, but it really isn't except for
a primary base, which ought to be handled by the normal process.
+guess_num_vtable_pointers (struct vtv_graph_node *class_node)
I would think it would be better to pass the unrounded count to the
library, and let the library decide how to adjust that number for
allocation.
+ /* Check array size, and re-size it if necessary. */
...
+ node->offsets = (unsigned *) xmalloc (10 * sizeof (unsigned));
Why not use VEC for your arrays?
+void __VLTunprotect (void) __attribute__ ((constructor(98)));
+void __VLTprotect (void) __attribute__ ((constructor(100)));
+
+void
+__VLTunprotect (void)
+{
+ __VLTChangePermission (__VLTP_READ_WRITE);
+}
+
+void
+__VLTprotect (void)
+{
+ __VLTChangePermission (__VLTP_READ_ONLY);
+}
You can add the attribute to the definitions by putting it before the
return type.
+ /* TODO: Temp fix. Needs to be tightened. */
+ if (num_vtable_map_nodes == 0)
+ return false;;
TODO?
+ var_name = ACONCAT (("_ZN4_VTVI", IDENTIFIER_POINTER (base_id),
+ "E12__vtable_mapE", NULL));
Please use the mangle.c machinery to generate mangled names. And add
demangling support to libiberty/cp-demangle.c.
+/* Be careful about changing this routine. The values generated will
+ be stored in the calls to InitSet. So, changing this routine may
+ cause a binary incompatibility. */
Making a particular hash function part of the ABI seems fragile.
+ /* 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. */
Well, yes; since you're going to be initializing it at run-time, it
isn't read-only. Same thing as with C++ const variables that need
dynamic initialization.
+ /* Let the garbabe collector collect the memory associated with the
"garbage"
+void
+vtv_save_class_info (tree record)
+{
+ if (!flag_vtable_verify || TREE_CODE (record) == UNION_TYPE)
+ return;
+
+ gcc_assert (TREE_CODE (record) == RECORD_TYPE);
+
+ vlt_saved_class_info = tree_cons (NULL_TREE, record, vlt_saved_class_info);
+}
This should really be an VEC, not a list.
+find_and_remove_next_leaf_node (struct work_node **worklist)
This needs to free removed work nodes.
+ if (! (DECL_NAME (TREE_OPERAND (rhs, 1)))
+ || (strncmp (IDENTIFIER_POINTER (DECL_NAME (TREE_OPERAND (rhs, 1))),
+ "_vptr.", 6) != 0))
You can use DECL_VIRTUAL_P to recognize the vptr FIELD_DECL.
+type_name_is_vtable_pointer (tree lhs)
And then you shouldn't need to check this at all.
+ ld = ggc_alloc_cleared_lang_decl (sizeof (struct lang_decl_fn));
lang_decl_fn is from the C++ front end, but you're not in the front end
here. It's probably simpler to use extern "C" for the library functions
like we do for other bits of libsupc++.
+reset_type_qualifiers (unsigned int new_quals, tree type_node)
This function is not safe and should be removed; as mentioned above, it
shouldn't be needed anyway.
+my_get_vtbl_decl_for_binfo (tree binfo)
You just copied get_vtbl_decl_for_binfo into the middle end. And the
two uses seem unnecessary: the first one seems redundant after checking
BINFO_VTABLE, and the second one seems redundant with getting the vtable
out of vtable_map_node.
+ /* Find the previousg statement that gets the "_vptr"
"previous"
+find_and_replace_var (gimple stmt, tree old_var, tree new_var)
+find_stmt_in_bb_stmts (gimple stmt, gimple_stmt_iterator *gsi_temp)
These seem like generally useful functions that should live somewhere else.
+ if (POINTER_TYPE_P (TREE_TYPE (vtbl)))
+ force_gimple_operand (vtbl, &pre_p, 1, NULL);
You aren't doing anything with the return value here. And it looks like
the vtbl variable is unused anyway.
- 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.
+ 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++.
+++ b/libstdc++-v3/libsupc++/vtv_map.h
Do you really need yet another hash table?
Jason
