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.
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.
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.
+ /* 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.
+ && (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.
+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?
+ 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.
Jason
