* [PATCH][C++] Fix ICE with VTV
@ 2019-02-19 9:57 Richard Biener
2019-02-19 16:54 ` Caroline Tice via gcc-patches
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2019-02-19 9:57 UTC (permalink / raw)
To: gcc-patches; +Cc: cmtice, jason
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.
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?
Thanks,
Richard.
2019-02-19 Richard Biener <rguenther@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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][C++] Fix ICE with VTV
2019-02-19 9:57 [PATCH][C++] Fix ICE with VTV Richard Biener
@ 2019-02-19 16:54 ` Caroline Tice via gcc-patches
2019-02-20 14:35 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Caroline Tice via gcc-patches @ 2019-02-19 16:54 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, Jason Merrill
On Tue, Feb 19, 2019 at 1:57 AM Richard Biener <rguenther@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.
>
> Thanks,
> Richard.
>
> 2019-02-19 Richard Biener <rguenther@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);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][C++] Fix ICE with VTV
2019-02-19 16:54 ` Caroline Tice via gcc-patches
@ 2019-02-20 14:35 ` Richard Biener
2019-02-20 17:58 ` Caroline Tice via gcc-patches
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2019-02-20 14:35 UTC (permalink / raw)
To: Caroline Tice; +Cc: GCC Patches, Jason Merrill
On Tue, 19 Feb 2019, Caroline Tice wrote:
> On Tue, Feb 19, 2019 at 1:57 AM Richard Biener <rguenther@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 <rguenther@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][C++] Fix ICE with VTV
2019-02-20 14:35 ` Richard Biener
@ 2019-02-20 17:58 ` Caroline Tice via gcc-patches
2019-02-21 9:57 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Caroline Tice via gcc-patches @ 2019-02-20 17:58 UTC (permalink / raw)
To: Richard Biener, GCC Patches; +Cc: Jason Merrill, Eric Christopher, Luis Lozano
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.
-- Caroline Tice
cmtice@google.com
On Wed, Feb 20, 2019 at 6:30 AM Richard Biener <rguenther@suse.de> wrote:
>
> On Tue, 19 Feb 2019, Caroline Tice wrote:
>
> > On Tue, Feb 19, 2019 at 1:57 AM Richard Biener <rguenther@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 <rguenther@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 <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][C++] Fix ICE with VTV
2019-02-20 17:58 ` Caroline Tice via gcc-patches
@ 2019-02-21 9:57 ` Richard Biener
0 siblings, 0 replies; 5+ messages in thread
From: Richard Biener @ 2019-02-21 9:57 UTC (permalink / raw)
To: Caroline Tice
Cc: GCC Patches, Jason Merrill, Eric Christopher, Luis Lozano, Jan Hubicka
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
> cmtice@google.com
>
> On Wed, Feb 20, 2019 at 6:30 AM Richard Biener <rguenther@suse.de> wrote:
> >
> > On Tue, 19 Feb 2019, Caroline Tice wrote:
> >
> > > On Tue, Feb 19, 2019 at 1:57 AM Richard Biener <rguenther@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 <rguenther@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 <rguenther@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-21 8:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-19 9:57 [PATCH][C++] Fix ICE with VTV Richard Biener
2019-02-19 16:54 ` Caroline Tice via gcc-patches
2019-02-20 14:35 ` Richard Biener
2019-02-20 17:58 ` Caroline Tice via gcc-patches
2019-02-21 9:57 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).