This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Vtable pointer verification, C++ front end changes (patch 1 of 3)
- From: Jason Merrill <jason at redhat dot com>
- To: Caroline Tice <cmtice at google dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Luis Lozano <llozano at google dot com>, Diego Novillo <dnovillo at google dot com>, Bhaskar Janakiraman <bjanakiraman at google dot com>
- Date: Fri, 01 Feb 2013 11:48:55 -0500
- Subject: Re: [PATCH] Vtable pointer verification, C++ front end changes (patch 1 of 3)
- References: <CABtf2+ROV47=LoN7v2=R9ef7WVpwZVhtax6TLUu4vrQ-R0Ci-A@mail.gmail.com> <510957BF.10306@redhat.com> <CABtf2+QvG8HFtArCOcmpzfSLvAdegEWE+KBZGJCZYDJDLraUFw@mail.gmail.com>
On 01/31/2013 07:24 PM, Caroline Tice wrote:
On Wed, Jan 30, 2013 at 9:26 AM, Jason Merrill <jason@redhat.com> wrote:
@@ -17954,6 +17954,10 @@ mark_class_instantiated (tree t, int ext
+ 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.
Why? instantiated templates also go through finish_struct_1. And we
only hit this function for explicit instantiations, not implicit.
+ 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.
Ah, I guess you don't want TYPE_LINKAGE_IDENTIFIER, as that's the simple
name rather than the mangled one. But for the external name you always
want to look at TYPE_NAME, not TREE_CHAIN (which corresponds to
TYPE_STUB_DECL); in the case of an anonymous class that gets a name for
linkage purposes from a typedef, the latter will have the original
placeholder name, while the former will have the name used in mangling.
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*.
Why? You are getting a qualified variant of the class somehow. Where
is it coming from?
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...
Yep, that's what I was thinking.
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).
TYPE_UID is already a property of the type, different from the class_uid
in your patch.
But yes, I guess you do need some way to get from your index back to the
type, so never mind.
+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.
I guess that makes sense.
+ var_name = ACONCAT (("_ZN4_VTVI", IDENTIFIER_POINTER (base_id),
+ "E12__vtable_mapE", NULL));
$ c++filt _ZN4_VTVISt13bad_exceptionE12__vtable_mapE
_VTV<std::bad_exception>::__vtable_map
Interesting. Does this _VTV template appear anywhere else?
Even if we stay with this approach to producing the name, I'd like it to
happen in a (new) function in mangle.c.
+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.
TYPE_MAIN_VARIANT will give you an unqualified variant of any qualified
type.
Jason