On Wed, Mar 13, 2013 at 10:48 AM, Diego Novillo <dnovi...@google.com> wrote:
>
> On 2013-02-25 14:27 , Caroline Tice wrote:
>
>> Index: libgcc/Makefile.in
>> ===================================================================
>> --- libgcc/Makefile.in (revision 196266)
>> +++ libgcc/Makefile.in (working copy)
>> @@ -22,6 +22,7 @@
>> libgcc_topdir = @libgcc_topdir@
>> host_subdir = @host_subdir@
>>
>> +gcc_srcdir = $(libgcc_topdir)/gcc
>> gcc_objdir = $(MULTIBUILDTOP)../../$(host_subdir)/gcc
>>
>> srcdir = @srcdir@
>> @@ -969,6 +970,16 @@ crtendS$(objext): $(srcdir)/crtstuff.c
>> # This is a version of crtbegin for -static links.
>> crtbeginT$(objext): $(srcdir)/crtstuff.c
>> $(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $< -DCRT_BEGIN -DCRTSTUFFT_O
>> +
>> +# These are used in vtable verification; see comments in source files for
>> +# more details.
>> +vtv_start$(objext): $(gcc_srcdir)/vtv_start.c
>> + $(crt_compile) $(CRTSTUFF_T_CFLAGS_S) \
>> + -c $(gcc_srcdir)/vtv_start.c
>> +
>> +vtv_end$(objext): $(gcc_srcdir)/vtv_end.c
>> + $(crt_compile) $(CRTSTUFF_T_CFLAGS_S) \
>> + -c $(gcc_srcdir)/vtv_end.c
>> endif
>
>
> Why not have these two files in libgcc? I don't think we want to depend on
> source files in gcc/ from libgcc/
>
I will put them there.
>>
>>
>> /* IPA Passes */
>> extern struct simple_ipa_opt_pass pass_ipa_lower_emutls;
>> Index: gcc/tree-vtable-verify.c
>> ===================================================================
>> --- gcc/tree-vtable-verify.c (revision 0)
>> +++ gcc/tree-vtable-verify.c (revision 0)
>
> I would like to get rid of this naming convention where we prefix file names
> with 'tree-'. Just vtable-verify.c is fine.
>
>> @@ -0,0 +1,724 @@
>> +/* Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
>
> Copyright (C) 2013
>
>>
>> +/* Virtual Table Pointer Security Pass - Detect corruption of vtable
>> pointers
>> + before using them for virtual method dispatches. */
>
>
> Do you have a URL to a paper/presentation for it?
No, I don't at this time.
>
>> +
>> + To find and reference the set of valid vtable pointers for any given
>> + virtual class, we create a special global varible for each virtual
>
> s/varible/variable/
>
>>
>> +
>> + Some implementation details for this pass:
>> +
>> + To find the all of the virtual calls, we iterate through all the
>
> s/the all/all/
>>
>>
>> + struct vtable_registration key;
>> + struct vtable_registration **slot;
>> +
>> + gcc_assert (node && node->registered);
>> +
>> + key.vtable_decl = vtable_decl;
>> + slot = (struct vtable_registration **) htab_find_slot (node->registered,
>> + &key, NO_INSERT);
>
> Unless you need to use this in an old branch, I strongly suggest using the
> new hash table facilities (hash-table.h).
>
Ok, will do.
>> +
>> +
>> +/* Here are the three two structures into which we insert vtable map nodes.
>
> 'three two'?
>>
>>
>> + /* Verify that there aren't any qualifiers on the type. */
>> + type_quals = TYPE_QUALS (TREE_TYPE (class_type_decl));
>> + gcc_assert (type_quals == TYPE_UNQUALIFIED);
>> +
>> + /* Get the mangled name for the unqualified type. */
>> + class_name = DECL_ASSEMBLER_NAME (class_type_decl);
>
>
> DECL_ASSEMBLER_NAME has side-effects (it generates one if there isn't one
> already). Just to avoid this unwanted side effect, protect it with if
> (HAS_DECL_ASSEMBLER_NAME_P). In fact, I think you should abort if the
> class_type_decl does *not* have one. So, just adding gcc_assert
> (HAS_DECL_ASSEMBLER_NAME_P(class_type_decl)) should be sufficient.
>
Will do.
>>
>> +
>> +/* This function attempts to recover the declared class of an object
>> + that is used in making a virtual call. We try to get the type from
>> + the type cast in the gimple assignment statement that extracts the
>> + vtable pointer from the object (DEF_STMT). The gimple statment
>
> s/statment/statement/
>>
>> + usually looks something like this:
>> +
>> + D.2201_4 = MEM[(struct Event *)this_1(D)]._vptr.Event */
>> +
>> +static tree
>> +/* extract_object_class_type (gimple def_stmt) */
>
> Remove this?
Yes (I didn't realize I had left the commented out code in the patch;
I'm sorry about that).
>>
>> +extract_object_class_type (tree rhs)
>> +{
>> + /* tree rhs = NULL_TREE; */
>> +
>> + /* Try to find and extract the type cast from that stmt. */
>> +
>> + /* rhs = gimple_assign_rhs1 (def_stmt); */
>> + /*
>> + if (TREE_CODE (rhs) == COMPONENT_REF)
>> + {
>> + while (TREE_CODE (rhs) == COMPONENT_REF
>> + && (TREE_CODE (TREE_OPERAND (rhs, 0)) == COMPONENT_REF))
>> + rhs = TREE_OPERAND (rhs, 0);
>> +
>> + if (TREE_CODE (rhs) == COMPONENT_REF
>> + && TREE_CODE (TREE_OPERAND (rhs, 0)) == MEM_REF
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (rhs, 0)))== RECORD_TYPE)
>> + rhs = TREE_TYPE (TREE_OPERAND (rhs, 0));
>> + else
>> + rhs = NULL_TREE;
>> + }
>> + else
>> + rhs = NULL_TREE;
>> + */
>
>
> What's this? Is it needed? There's some other commented code here that
> seems out of place.
>
I will remove it. I didn't mean for it to be in the patch.
>> +
>> + tree result = NULL_TREE;
>> +
>> +
>> + if (TREE_CODE (rhs) == COMPONENT_REF)
>> + {
>> + tree op0 = TREE_OPERAND (rhs, 0);
>> + tree op1 = TREE_OPERAND (rhs, 1);
>> +
>> + if (TREE_CODE (op1) == FIELD_DECL
>> + && DECL_VIRTUAL_P (op1))
>> + {
>> + if (TREE_CODE (op0) == COMPONENT_REF
>> + && TREE_CODE (TREE_OPERAND (op0, 0)) == MEM_REF
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (op0, 0)))== RECORD_TYPE)
>> + result = TREE_TYPE (TREE_OPERAND (op0, 0));
>> + else
>> + result = TREE_TYPE (op0);
>> + }
>> + else if (TREE_CODE (op0) == COMPONENT_REF)
>> + {
>> + result = extract_object_class_type (op0);
>> + if (result == NULL_TREE
>> + && TREE_CODE (op1) == COMPONENT_REF)
>> + result = extract_object_class_type (op1);
>> + }
>> + }
>
> Indentation seems off here.
>>
>> +
>> + return result;
>> +}
>> +
>> +bool
>> +vtv_defuse_fn (tree var, gimple def_stmt, void *data)
>> +{
>> + bool retval = false;
>> +
>> + return retval;
>> +}
>
>
> Huh?
>
Another piece I meant to remove previously (sorry again).
>>
>> + for (; !gsi_end_p (gsi_virtual_call); gsi_next (&gsi_virtual_call))
>> + {
>> + stmt = gsi_stmt (gsi_virtual_call);
>> + if (is_vtable_assignment_stmt (stmt))
>> + {
>> + tree lhs = gimple_assign_lhs (stmt);
>> + tree vtbl_var_decl = NULL_TREE;
>> + struct vtbl_map_node *vtable_map_node;
>> + tree vtbl_decl = NULL_TREE;
>> + gimple call_stmt;
>> + struct gimplify_ctx gctx;
>> + const char *vtable_name = "<unknown>";
>> + tree tmp0;
>> + bool found;
>> +
>> + gsi_vtbl_assign = gsi_for_stmt (stmt);
>
> Why not use 'gsi_virtual_call'?
>
I really do want to use two separate iterators here. gsi_virtual_call
iterates linearly through all the statements in the basic block.
gsi_vtbl_assign gets moved around a bit, later down in the code, to
find the exact position for inserting the verification call.
>>
>> + if (TREE_CODE (vtbl_decl) == VAR_DECL)
>> + vtable_name = IDENTIFIER_POINTER (DECL_NAME
>> (vtbl_decl));
>> +
>> + push_gimplify_context (&gctx);
>
> Not needed. You are not calling the gimplifier.
>
Ok.
>>
>> +
>> + if (!next_stmt)
>> + {
>> + pop_gimplify_context (NULL);
>
> Likewise.
Ok.
>
>>
>> + gsi_insert_after (&gsi_vtbl_assign, call_stmt,
>> + GSI_NEW_STMT);
>> +
>> + pop_gimplify_context (NULL);
>
> Likewise.
>
Ok.
>
> Diego.
