This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Fwd: [PATCH, updated] Vtable pointer verification, C++ front end changes (patch 1 of 3)


Trying to send again; gcc_patches list bounced original message.

---------- Forwarded message ----------
From: Caroline Tice <cmtice@google.com>
Date: Fri, May 24, 2013 at 9:15 AM
Subject: Re: [PATCH, updated] Vtable pointer verification, C++ front
end changes (patch 1 of 3)
To: Jason Merrill <jason@redhat.com>
Cc: Diego Novillo <dnovillo@google.com>, Luis Lozano
<llozano@google.com>, Bhaskar Janakiraman <bjanakiraman@google.com>,
GCC Patches <gcc-patches@gcc.gnu.org>



I'm afraid I have been busy trying to finish up other things recently,
but now I am ready to focus full time on this again.

On Mon, Apr 8, 2013 at 6:21 PM, Jason Merrill <jason@redhat.com> wrote:
>
> 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.
>

Ok, I will remove this.

>>
>> +         information aboui which vtable will actually be emitted.  */
>
>
> "about"


Done.

>
>
>> +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.
>

I stopped using finish_objects because it always called
expand_or_defer_fn, and I did not want that to happen here.

>>>
>>> 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.
>

It seems correct to only update the g++ driver, since this option only
does things for C++ programs.  I'm not sure what address sanitizer
does "directly in the specs"...Which specs would these be?


>>>
>>> +       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.
>

Originally I was not doing anything with the specs, and so I didn't
want to have to add a special link command, to link in a special
library whenever the fvtable-verify option is used.  Now that I've
started changing the specs anyway, I suppose I could go ahead and put
this in its own separate library and add a link option.  Is that
really the way I should go with this?

>>>>
>>>> -      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 am sorry to disagree with you, but you are incorrect.  The second
change can be eliminated (the one that adds SECTION_LINKONCE to
flags), but the first change above is necessary.  What the first bit
of code does is to ensure that each vtable map variable gets its own
unique comdat name.  When I tried removing this code, it put all the
vtable map variables into the same section with a single comdat name.
This will not work for us because the linker may need to pick and
choose individual variables out of each object file, not accept or
reject all the variables in an object file as a single group.


>>
>> 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.
>

The section name code is still required, but I can eliminate the
DECL_COMDAT_GROUP assignment and replace it with a call to
comdat_linkage.


>>
>> +/* 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.
>

Ok, I will fix the comments.

>>
>> +  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.
>

Thanks for pointing that out (I was unaware of that).  I will happily
use the pre-built nodes.

>>
>> +  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.
>
Will do.

>>
>> +  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.
>
Ok, I will use that.

>>
>> +   data.  VPTR_ADDRESS is and expression for calculating the correct
>
>
> "an"
>
Fixed.

>
> >        /* 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.
>

Ok.

>>
>> +              /* 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.
>

I will update the 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".
>

I will update the comment.

>>
>> +          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.
>

OK.

>>
>> +          && (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.


When I tried removing that condition, my thunk test failed.  When I
add it back, the test passes again.

>
>
>> +      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?
>

Will do.

>>
>> +          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?
>

Whoops, old code sneaking in.  They should both just be using
TREE_ASM_WRITTEN. I will fix that.

>>
>> +              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.
>

Ok I will remove that test.

>>
>> +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?
>


This inserts a reference to a symbol that will only be resolved if the
proper libraries are linked in.  It is a way to make sure that, if
some library did not get linked in properly, we get a link time
failure rather than a run time failure.

>>
>> +          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.
>

Will do.

>>
>> +       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.
>


If I omit allocate_struct_function and do nothing else, I get an ICE
in gimplify_function_tree (a few lines further down).   (It actually
fails on an assert from push_cfun, which is called from
gimplify_function_tree).

If I replace the call to allocate_struct_function above with the
push_cfun call you suggest above, I get  the same assertion failure,
this time from my call...The   assertion is:

  gcc_assert ((!cfun && !current_function_decl)
              || (cfun && current_function_decl == cfun->decl));

(gdb) up
#1  0x0000000000a2e2cf in push_cfun (new_cfun=0x7ffff61f3be0)
    at ../../gcc-fsf/gcc/function.c:4435
4435  gcc_assert ((!cfun && !current_function_decl)
(gdb) p cfun
$1 = (function *) 0x0
(gdb) p current_function_decl
$2 = (tree) 0x7ffff61e8200
(gdb)


Therefore I believe you are incorrect, and that my call to
allocate_struct_function is necessary.


>> +      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.
>

Will do.

>>
>> +      /* 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.
>

Will do.

>>
>> +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 rename vtv_save_base_class_info to make it less confusing.


>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]