On Wed, 20 Feb 2019, Caroline Tice wrote: > I have managed to reproduce the issue now, and the patch does appear > to fix the ICE. > > There is a second issue, which will need more investigation: When > compiled with '-flto' the extra internal functions that VTV generates, > and which are necessary for it to work correctly, do not get > propagated into the final binary. You can see this compiling the > bb_tests.cc test case in the libvtv testsuite, and grepping for > 'GLOBAL' in the final output: > > $ /usr/local/google3/cmtice/gcc-fsf.root/usr/local/bin/g++ -o bb_tests > bb_tests.cc -O1 -fvtable-verify=std // Compile without flto > $ nm bb_tests | grep GLOBAL > 0000000000601000 d _GLOBAL_OFFSET_TABLE_ > 0000000000400930 t _GLOBAL__sub_I.00099__Z14get_cond_value > > $ /usr/local/google3/cmtice/gcc-fsf.root/usr/local/bin/g++ -o > bb_tests_flto bb_tests.cc -O1 -flto -fvtable-verify=std // Compile > with flto > $ nm bb_tests_flto | grep GLOBAL > 0000000000601000 d _GLOBAL_OFFSET_TABLE_ > > But as said, that's a separate issue, which I will need to investigate > (if anyone has any suggestions as to the proper way to propagate the > functions through -flto, I would love to hear them). > > I approve Richard's patch for fixing the ICE.
I have applied the patch. I do not see LTO reclaiming any symbol when compiling bb_tests.cc but of course it might bring symbols local and/or inline them. Did you verify that there is a functional deficiency rather than just the missing symbol table entry? That is, the bb_tests.cc testcase executes just fine. Richard. > -- Caroline Tice > cmt...@google.com > > On Wed, Feb 20, 2019 at 6:30 AM Richard Biener <rguent...@suse.de> wrote: > > > > On Tue, 19 Feb 2019, Caroline Tice wrote: > > > > > On Tue, Feb 19, 2019 at 1:57 AM Richard Biener <rguent...@suse.de> wrote: > > > > > > > > > > > Looks like vtv_generate_init_routine calls symtab->process_new_functions > > > > () which has seriously bad effects on GCCs internal memory state. > > > > > > > > VTV has zero testsuite coverage it seems and besides its initial > > > > commit didn't receive any love. > > > > > > > > > > I am puzzled by these statements, as neither of them is true. VTV does > > > have a testsuite in the libvtv directory. Naturally you can only run them > > > when vtv is enabled and only from the libvtv build tree, as they will all > > > fail if VTV is not enabled. I have fixed various bugs in VTV since the > > > initial commit, and I have also approved several patches, especially for > > > people migrating it to other architectures, suggesting that interest in it > > > is not zero. > > > > > > > > > > > > > > > > > Can we rip it out please? > > > > > > > > Is the patch OK? (Pointless) bootstrap and regtest running on > > > > x86_64-unknown-linux-gnu. > > > > > > > > Has anybody "recently" tried to enable the feature and tested the > > > > result? > > > > > > > > > > I have not tried building it recently, but will do so. I will review your > > > patch to see if it makes sense. I would prefer that VTV not be 'ripped > > > out' of GCC, and will do whatever I can, within reason, to try to fix its > > > issues. > > > > Meanwhile the patch passed bootstrap and testing with > > --enable-vtable-verify. > > > > Richard. > > > > > > > > > > > > > Thanks, > > > > Richard. > > > > > > > > 2019-02-19 Richard Biener <rguent...@suse.de> > > > > > > > > PR middle-end/89392 > > > > cp/ > > > > * vtable-class-hierarchy.c (vtv_generate_init_routine): Do not > > > > make symtab process new functions here. > > > > > > > > Index: gcc/cp/vtable-class-hierarchy.c > > > > =================================================================== > > > > --- gcc/cp/vtable-class-hierarchy.c (revision 269009) > > > > +++ gcc/cp/vtable-class-hierarchy.c (working copy) > > > > @@ -1191,8 +1191,6 @@ vtv_generate_init_routine (void) > > > > gimplify_function_tree (vtv_fndecl); > > > > cgraph_node::add_new_function (vtv_fndecl, false); > > > > > > > > - symtab->process_new_functions (); > > > > - > > > > if (flag_vtable_verify == VTV_PREINIT_PRIORITY && !TARGET_PECOFF) > > > > assemble_vtv_preinit_initializer (vtv_fndecl); > > > > > > > > > > > > > > > -- > > Richard Biener <rguent...@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB > > 21284 (AG Nuernberg) > > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
