On Wed, Jan 30, 2013 at 9:26 AM, Jason Merrill <ja...@redhat.com> wrote:
> 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?
>
If we don't have this in both places, then we miss getting vtable
pointers for instantiated templates.
>> -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.
You are right; this is leftover from an earlier version where we were
calling this function directly; I'll remove the change.
>
>> 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.
Exactly. We need to know which vtables are really emitted before we
output __VLTRegisterPair calls; otherwise we got in trouble emitting
calls for vtables that weren't output, or causing vtables to be output
when otherwise they wouldn't be.
>
>> + 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?
>
Whoops. This is leftover from older code when we modified
start_objects and added that to the constructor initialization
function. I will change this.
>> + 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 know the difference between DECL_ASSEMBLER_NAME and
TYPE_LINKAGE_IDENTIFIER. We are just trying to get the mangled name
for the class.
> 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.
>
We used to not have the "qualifier business", assuming that classes
did not have any type qualifiers. This turned out not to be a true
assumption. Occasionally we were getting a case where a class had a
"const" qualifier attached to it *sometimes*. We used the mangled
name to look it up in our hashtable, and it didn't find the entry for
the class (which had been put in without the const qualifier). So we
temporarily remove any type qualifiers before getting the mangled
name; then re-apply whatever qualifiers were there before. The cod
actually works because the type_decl_type we change the qualifiers on
is a subtree of the class_type_decl, which is a subtree of the
base_class that we get the mangled name from. (We have tested this
code; it works and fixes our problem).
>> + 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.
>
As written the insert function doesn't return anything to let you know
whether the item was already there or not, which we need to know (we
use the results here to avoid generating redundant calls to
__VLTRegisterPair. I suppose we could modify the insert function to
return a boolean indicating if the item was already in the hashtable,
and then we could get by with just one call here...
>
>> 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?
>
I suppose we could eliminate the linked list...
> 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.
>
I don't understand how this would work. I think we need the vec, at
least, to have direct access based on TYPE_UID (which is also the vec
index).
>
>> +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.
If there is any computation we can do at compile-time rather than
run-time, we would rather do it at compile time.
>
>> + /* Check array size, and re-size it if necessary. */
>
> ...
>>
>> + node->offsets = (unsigned *) xmalloc (10 * sizeof (unsigned));
>
>
> Why not use VEC for your arrays?
>
Will do.
>> + 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.
>
I don't think the mangle.c machinery can generate the mangled name
that we need. We did construct this mangled name for our vtable map
variables very carefully, so that it does actually demangle properly,
with no additional support needed.
For example, one of the vtable map variable names generated by the
code above is:
_ZN4_VTVISt13bad_exceptionE12__vtable_mapE
$ c++filt _ZN4_VTVISt13bad_exceptionE12__vtable_mapE
_VTV<std::bad_exception>::__vtable_map
$
Is this really not OK?
>> +/* 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.
>
Again, while I understand your concern, we are VERY concerned about
runtime performance, and we want to push any computations that we can
to compile-time.
>> +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.
>
As I explained above, we originally didn't have it and then found we
really needed it. If you know of a safer or better way to accomplish
the same thing we would be happy to hear about it.
>> - 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.
>> +++ b/libstdc++-v3/libsupc++/vtv_map.h
>
>
> Do you really need yet another hash table?
>
Yes, sadly, we really do.
