| Message ID | 668a984e.050a0220.58cf7.20c1@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: Conditionally start timer during lazy load [PR115165] | expand |
On 7/7/24 9:29 AM, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > Or should I include a testcase? I haven't reduced one from using the > full contents of C++23 <iostream> yet but I can do so if you prefer. Would adding -ftime-report to the pr99166 test reproduce the issue? > -- >8 -- > > While lazy loading, instantiation of pendings can sometimes recursively > perform name lookup and begin further lazy loading. When using the > '-ftime-report' functionality this causes ICEs as we could start an > already-running timer for the importing. > > This patch fixes the issue by using the 'timevar_cond*' API instead to > support such recursive calls. > > PR c++/115165 > > gcc/cp/ChangeLog: > > * module.cc (lazy_load_binding): Use 'timevar_cond*' APIs. > (lazy_load_pendings): Likewise. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index dc5d046f04d..fec1b7e58df 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -19564,7 +19564,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) > { > int count = errorcount + warningcount; > > - timevar_start (TV_MODULE_IMPORT); > + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); > > /* Make sure lazy loading from a template context behaves as if > from a non-template context. */ > @@ -19594,7 +19594,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) > > function_depth--; > > - timevar_stop (TV_MODULE_IMPORT); > + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); > > if (!ok) > fatal_error (input_location, > @@ -19633,7 +19633,7 @@ lazy_load_pendings (tree decl) > > int count = errorcount + warningcount; > > - timevar_start (TV_MODULE_IMPORT); > + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); > bool ok = !recursive_lazy (); > if (ok) > { > @@ -19667,7 +19667,7 @@ lazy_load_pendings (tree decl) > function_depth--; > } > > - timevar_stop (TV_MODULE_IMPORT); > + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); > > if (!ok) > fatal_error (input_location, "failed to load pendings for %<%E%s%E%>",
On Wed, Jul 17, 2024 at 04:31:21PM -0400, Jason Merrill wrote: > On 7/7/24 9:29 AM, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > Or should I include a testcase? I haven't reduced one from using the > > full contents of C++23 <iostream> yet but I can do so if you prefer. > > Would adding -ftime-report to the pr99166 test reproduce the issue? > Unfortunately not, since lazy loading is not triggered in this test. I've ended up just reducing a testcase anyway, here's a new version of the patch with a test included (that fails beforehand and not after); OK for trunk? (And even 14, actually?) -- >8 -- While lazy loading, instantiation of pendings can sometimes recursively perform name lookup and begin further lazy loading. When using the '-ftime-report' functionality this causes ICEs as we could start an already-running timer for the importing. This patch fixes the issue by using the 'timevar_cond*' API instead to support such recursive calls. PR c++/115165 gcc/cp/ChangeLog: * module.cc (lazy_load_binding): Use 'timevar_cond*' APIs. (lazy_load_pendings): Likewise. gcc/testsuite/ChangeLog: * g++.dg/modules/timevar-1_a.H: New test. * g++.dg/modules/timevar-1_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 8 ++++---- gcc/testsuite/g++.dg/modules/timevar-1_a.H | 14 ++++++++++++++ gcc/testsuite/g++.dg/modules/timevar-1_b.C | 10 ++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/timevar-1_a.H create mode 100644 gcc/testsuite/g++.dg/modules/timevar-1_b.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index d385b422168..69764fd772d 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -19604,7 +19604,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) { int count = errorcount + warningcount; - timevar_start (TV_MODULE_IMPORT); + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); /* Make sure lazy loading from a template context behaves as if from a non-template context. */ @@ -19634,7 +19634,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) function_depth--; - timevar_stop (TV_MODULE_IMPORT); + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); if (!ok) fatal_error (input_location, @@ -19673,7 +19673,7 @@ lazy_load_pendings (tree decl) int count = errorcount + warningcount; - timevar_start (TV_MODULE_IMPORT); + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); bool ok = !recursive_lazy (); if (ok) { @@ -19707,7 +19707,7 @@ lazy_load_pendings (tree decl) function_depth--; } - timevar_stop (TV_MODULE_IMPORT); + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); if (!ok) fatal_error (input_location, "failed to load pendings for %<%E%s%E%>", diff --git a/gcc/testsuite/g++.dg/modules/timevar-1_a.H b/gcc/testsuite/g++.dg/modules/timevar-1_a.H new file mode 100644 index 00000000000..efd7f0ecced --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/timevar-1_a.H @@ -0,0 +1,14 @@ +// PR c++/115165 +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +template <typename> struct A { virtual ~A(); }; +struct B : A<char> {}; +struct C : B { C() {} }; + +class D { C c; }; +void f(D); + +struct X { + friend void f(X); +}; diff --git a/gcc/testsuite/g++.dg/modules/timevar-1_b.C b/gcc/testsuite/g++.dg/modules/timevar-1_b.C new file mode 100644 index 00000000000..645f01697eb --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/timevar-1_b.C @@ -0,0 +1,10 @@ +// PR c++/115165 +// { dg-additional-options "-fmodules-ts -ftime-report" } +// { dg-allow-blank-lines-in-output 1 } +// { dg-prune-output "Time variable" } +// { dg-prune-output "\[0-9\]+%" } +// { dg-prune-output "TOTAL" } +// { dg-prune-output "checks" } + +import "timevar-1_a.H"; +X x;
On 7/17/24 10:36 PM, Nathaniel Shead wrote: > On Wed, Jul 17, 2024 at 04:31:21PM -0400, Jason Merrill wrote: >> On 7/7/24 9:29 AM, Nathaniel Shead wrote: >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >>> >>> Or should I include a testcase? I haven't reduced one from using the >>> full contents of C++23 <iostream> yet but I can do so if you prefer. >> >> Would adding -ftime-report to the pr99166 test reproduce the issue? >> > > Unfortunately not, since lazy loading is not triggered in this test. > > I've ended up just reducing a testcase anyway, here's a new version of > the patch with a test included (that fails beforehand and not after); OK > for trunk? (And even 14, actually?) OK for both. > -- >8 -- > > While lazy loading, instantiation of pendings can sometimes recursively > perform name lookup and begin further lazy loading. When using the > '-ftime-report' functionality this causes ICEs as we could start an > already-running timer for the importing. > > This patch fixes the issue by using the 'timevar_cond*' API instead to > support such recursive calls. > > PR c++/115165 > > gcc/cp/ChangeLog: > > * module.cc (lazy_load_binding): Use 'timevar_cond*' APIs. > (lazy_load_pendings): Likewise. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/timevar-1_a.H: New test. > * g++.dg/modules/timevar-1_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 8 ++++---- > gcc/testsuite/g++.dg/modules/timevar-1_a.H | 14 ++++++++++++++ > gcc/testsuite/g++.dg/modules/timevar-1_b.C | 10 ++++++++++ > 3 files changed, 28 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/timevar-1_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/timevar-1_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index d385b422168..69764fd772d 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -19604,7 +19604,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) > { > int count = errorcount + warningcount; > > - timevar_start (TV_MODULE_IMPORT); > + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); > > /* Make sure lazy loading from a template context behaves as if > from a non-template context. */ > @@ -19634,7 +19634,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) > > function_depth--; > > - timevar_stop (TV_MODULE_IMPORT); > + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); > > if (!ok) > fatal_error (input_location, > @@ -19673,7 +19673,7 @@ lazy_load_pendings (tree decl) > > int count = errorcount + warningcount; > > - timevar_start (TV_MODULE_IMPORT); > + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); > bool ok = !recursive_lazy (); > if (ok) > { > @@ -19707,7 +19707,7 @@ lazy_load_pendings (tree decl) > function_depth--; > } > > - timevar_stop (TV_MODULE_IMPORT); > + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); > > if (!ok) > fatal_error (input_location, "failed to load pendings for %<%E%s%E%>", > diff --git a/gcc/testsuite/g++.dg/modules/timevar-1_a.H b/gcc/testsuite/g++.dg/modules/timevar-1_a.H > new file mode 100644 > index 00000000000..efd7f0ecced > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/timevar-1_a.H > @@ -0,0 +1,14 @@ > +// PR c++/115165 > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi {} } > + > +template <typename> struct A { virtual ~A(); }; > +struct B : A<char> {}; > +struct C : B { C() {} }; > + > +class D { C c; }; > +void f(D); > + > +struct X { > + friend void f(X); > +}; > diff --git a/gcc/testsuite/g++.dg/modules/timevar-1_b.C b/gcc/testsuite/g++.dg/modules/timevar-1_b.C > new file mode 100644 > index 00000000000..645f01697eb > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/timevar-1_b.C > @@ -0,0 +1,10 @@ > +// PR c++/115165 > +// { dg-additional-options "-fmodules-ts -ftime-report" } > +// { dg-allow-blank-lines-in-output 1 } > +// { dg-prune-output "Time variable" } > +// { dg-prune-output "\[0-9\]+%" } > +// { dg-prune-output "TOTAL" } > +// { dg-prune-output "checks" } > + > +import "timevar-1_a.H"; > +X x;
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index dc5d046f04d..fec1b7e58df 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -19564,7 +19564,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) { int count = errorcount + warningcount; - timevar_start (TV_MODULE_IMPORT); + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); /* Make sure lazy loading from a template context behaves as if from a non-template context. */ @@ -19594,7 +19594,7 @@ lazy_load_binding (unsigned mod, tree ns, tree id, binding_slot *mslot) function_depth--; - timevar_stop (TV_MODULE_IMPORT); + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); if (!ok) fatal_error (input_location, @@ -19633,7 +19633,7 @@ lazy_load_pendings (tree decl) int count = errorcount + warningcount; - timevar_start (TV_MODULE_IMPORT); + bool timer_running = timevar_cond_start (TV_MODULE_IMPORT); bool ok = !recursive_lazy (); if (ok) { @@ -19667,7 +19667,7 @@ lazy_load_pendings (tree decl) function_depth--; } - timevar_stop (TV_MODULE_IMPORT); + timevar_cond_stop (TV_MODULE_IMPORT, timer_running); if (!ok) fatal_error (input_location, "failed to load pendings for %<%E%s%E%>",
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? Or should I include a testcase? I haven't reduced one from using the full contents of C++23 <iostream> yet but I can do so if you prefer. -- >8 -- While lazy loading, instantiation of pendings can sometimes recursively perform name lookup and begin further lazy loading. When using the '-ftime-report' functionality this causes ICEs as we could start an already-running timer for the importing. This patch fixes the issue by using the 'timevar_cond*' API instead to support such recursive calls. PR c++/115165 gcc/cp/ChangeLog: * module.cc (lazy_load_binding): Use 'timevar_cond*' APIs. (lazy_load_pendings): Likewise. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)