On Tue, Jun 4, 2013 at 10:20 AM, Jason Merrill <ja...@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 cmt...@google.com > Jason >
