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

[Caroline Tice <cmtice at google dot com>]

On Tue, Jun 4, 2013 at 10:20 AM, Jason Merrill <jason@redhat.com> wrote:
> On 05/24/2013 12:15 PM, Caroline Tice wrote:
>>
>>         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.
>
>
> But people don't always compile C++ programs with the g++ driver; the gcc
> driver works fine for programs that don't require libstdc++.
>
>
>> I'm not sure what address sanitizer does
>> "directly in the specs"...Which specs would these be?
>
>
> SANITIZER_EARLY_SPEC and SANITIZER_SPEC in gcc.c.


Ok, I will make the spec changes directly in gcc.cc

>
>
>> 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?
>
>
> I think so, yes.
>

Will do.

>
>>             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.
>
>
> Ah, I see.  Then that's a bug that ought to be fixed: resolve_unique_section
> needs to deal better with decls with both DECL_SECTION_NAME and
> DECL_ONE_ONLY set.
>
> But I guess for now let's leave your code as it is with a FIXME note.
>

Ok, will do.

>
>>         +                  /* 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.
>
>
> Are you sure that's the right fix?  What is the purpose of this code? The
> comment sounds like you want to strip away an offset relative to a vtable
> pointer and get the actual vtable.  Testing for length 1 won't accomplish
> that for you.
>
>

The test for length 1 was just making sure the tree operand existed
before trying to access it; but I will rewrite this so it is less
confusing/misleading.

>>         +          && (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.
>
>
> Failed how?  I'm puzzled.  All that check could do would be to avoid
> registering a construction vtable, which should be harmless anyway.
>

I looked at this again; it was causing a failure because of "operator
error" on my part when I tried removing it; I only removed part of the
clause, and this test was always failing, causing the overall negated
clause to always pass; when I removed that clause then the overall
negated clause sometimes failed, which resulted in a vtable pointer
not being added to my set.  Removing the entire negated clause removes
the unnecessary check and does not cause any new test failures.
Bottom line: I can remove the test that you were complaining about and
still get things to work properly.


>>         +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.
>
>
> Won't the references to the registration functions also cause link time
> failures?

No, because there were no other link time references (the library was
always being linked in with --whole-archive for just that reason).
But I have found a different mechanism for doing this and the vtv_init
library is going away altogether (along with this undefined reference
variable).

>
>>         +       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).
>
>
> I think this is breaking because you set current_function_decl.  It ought to
> work better if you leave it unset and let gimplify_function_tree set it for
> you.
>

Yes, that works. Thanks!

-- Caroline
cmtice@google.com

> Jason
>