| Message ID | 65cc18c2.a70a0220.1edb0.766a@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] c++: Defer emitting inline variables [PR113708] | expand |
On 2/13/24 20:34, Nathaniel Shead wrote: > On Tue, Feb 13, 2024 at 06:08:42PM -0500, Jason Merrill wrote: >> On 2/11/24 08:26, Nathaniel Shead wrote: >>> >>> Currently inline vars imported from modules aren't correctly finalised, >>> which means that import_export_decl gets called at the end of TU >>> processing despite not being meant to for these kinds of declarations. >> >> I disagree that it's not meant to; inline variables are vague linkage just >> like template instantiations, so the bug seems to be that import_export_decl >> doesn't accept them. And on the other side, that make_rtl_for_nonlocal_decl >> doesn't defer them like instantations. >> >> Jason >> > > True, that's a good point. I think I confused myself here. > > Here's a fixed patch that looks a lot cleaner. Bootstrapped and > regtested (so far just dg.exp and modules.exp) on x86_64-pc-linux-gnu, > OK for trunk if full regtest succeeds? OK. > -- >8 -- > > Inline variables are vague-linkage, and may or may not need to be > emitted in any TU that they are part of, similarly to e.g. template > instantiations. > > Currently 'import_export_decl' assumes that inline variables have > already been emitted when it comes to end-of-TU processing, and so > crashes when importing non-trivially-initialised variables from a > module, as they have not yet been finalised. > > This patch fixes this by ensuring that inline variables are always > deferred till end-of-TU processing, unifying the behaviour for module > and non-module code. > > PR c++/113708 > > gcc/cp/ChangeLog: > > * decl.cc (make_rtl_for_nonlocal_decl): Defer inline variables. > * decl2.cc (import_export_decl): Support inline variables. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/init-7_a.H: New test. > * g++.dg/modules/init-7_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/decl.cc | 4 ++++ > gcc/cp/decl2.cc | 7 +++++-- > gcc/testsuite/g++.dg/modules/init-7_a.H | 6 ++++++ > gcc/testsuite/g++.dg/modules/init-7_b.C | 6 ++++++ > 4 files changed, 21 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/init-7_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/init-7_b.C > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 3e41fd4fa31..969513c069a 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -7954,6 +7954,10 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec) > && DECL_IMPLICIT_INSTANTIATION (decl)) > defer_p = 1; > > + /* Defer vague-linkage variables. */ > + if (DECL_INLINE_VAR_P (decl)) > + defer_p = 1; > + > /* If we're not deferring, go ahead and assemble the variable. */ > if (!defer_p) > rest_of_decl_compilation (decl, toplev, at_eof); > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index f569d4045ec..1dddbaab38b 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -3360,7 +3360,9 @@ import_export_decl (tree decl) > > * implicit instantiations of function templates > > - * inline function > + * inline functions > + > + * inline variables > > * implicit instantiations of static data members of class > templates > @@ -3383,6 +3385,7 @@ import_export_decl (tree decl) > || DECL_DECLARED_INLINE_P (decl)); > else > gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl) > + || DECL_INLINE_VAR_P (decl) > || DECL_VTABLE_OR_VTT_P (decl) > || DECL_TINFO_P (decl)); > /* Check that a definition of DECL is available in this translation > @@ -3511,7 +3514,7 @@ import_export_decl (tree decl) > this entity as undefined in this translation unit. */ > import_p = true; > } > - else if (DECL_FUNCTION_MEMBER_P (decl)) > + else if (TREE_CODE (decl) == FUNCTION_DECL && DECL_FUNCTION_MEMBER_P (decl)) > { > if (!DECL_DECLARED_INLINE_P (decl)) > { > diff --git a/gcc/testsuite/g++.dg/modules/init-7_a.H b/gcc/testsuite/g++.dg/modules/init-7_a.H > new file mode 100644 > index 00000000000..7a0bb721c30 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/init-7_a.H > @@ -0,0 +1,6 @@ > +// PR c++/113708 > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi {} } > + > +inline int f() { return 42; } > +inline int a = f(); > diff --git a/gcc/testsuite/g++.dg/modules/init-7_b.C b/gcc/testsuite/g++.dg/modules/init-7_b.C > new file mode 100644 > index 00000000000..58bb0620ca5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/init-7_b.C > @@ -0,0 +1,6 @@ > +// PR c++/113708 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts" } > + > +import "init-7_a.H"; > +int main() { a; }
On Tue, Feb 13, 2024 at 09:47:27PM -0500, Jason Merrill wrote: > On 2/13/24 20:34, Nathaniel Shead wrote: > > On Tue, Feb 13, 2024 at 06:08:42PM -0500, Jason Merrill wrote: > > > On 2/11/24 08:26, Nathaniel Shead wrote: > > > > > > > > Currently inline vars imported from modules aren't correctly finalised, > > > > which means that import_export_decl gets called at the end of TU > > > > processing despite not being meant to for these kinds of declarations. > > > > > > I disagree that it's not meant to; inline variables are vague linkage just > > > like template instantiations, so the bug seems to be that import_export_decl > > > doesn't accept them. And on the other side, that make_rtl_for_nonlocal_decl > > > doesn't defer them like instantations. > > > > > > Jason > > > > > > > True, that's a good point. I think I confused myself here. > > > > Here's a fixed patch that looks a lot cleaner. Bootstrapped and > > regtested (so far just dg.exp and modules.exp) on x86_64-pc-linux-gnu, > > OK for trunk if full regtest succeeds? > > OK. > A full bootstrap failed two tests in dwarf2.exp, which seem to be caused by an unreferenced 'inline' variable not being emitted into the debug info and thus causing the checks for its existence to fail. Adding a reference to the vars cause the tests to pass. Now fully bootstrapped and regtested on x86_64-pc-linux-gnu, still OK for trunk? (Only change is the two adjusted testcases.) -- >8 -- Inline variables are vague-linkage, and may or may not need to be emitted in any TU that they are part of, similarly to e.g. template instantiations. Currently 'import_export_decl' assumes that inline variables have already been emitted when it comes to end-of-TU processing, and so crashes when importing non-trivially-initialised variables from a module, as they have not yet been finalised. This patch fixes this by ensuring that inline variables are always deferred till end-of-TU processing, unifying the behaviour for module and non-module code. PR c++/113708 gcc/cp/ChangeLog: * decl.cc (make_rtl_for_nonlocal_decl): Defer inline variables. * decl2.cc (import_export_decl): Support inline variables. gcc/testsuite/ChangeLog: * g++.dg/debug/dwarf2/inline-var-1.C: Reference 'a' to ensure it is emitted. * g++.dg/debug/dwarf2/inline-var-3.C: Likewise. * g++.dg/modules/init-7_a.H: New test. * g++.dg/modules/init-7_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/decl.cc | 4 ++++ gcc/cp/decl2.cc | 7 +++++-- gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C | 2 ++ gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C | 2 ++ gcc/testsuite/g++.dg/modules/init-7_a.H | 6 ++++++ gcc/testsuite/g++.dg/modules/init-7_b.C | 6 ++++++ 6 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/init-7_a.H create mode 100644 gcc/testsuite/g++.dg/modules/init-7_b.C diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index 3e41fd4fa31..969513c069a 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -7954,6 +7954,10 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec) && DECL_IMPLICIT_INSTANTIATION (decl)) defer_p = 1; + /* Defer vague-linkage variables. */ + if (DECL_INLINE_VAR_P (decl)) + defer_p = 1; + /* If we're not deferring, go ahead and assemble the variable. */ if (!defer_p) rest_of_decl_compilation (decl, toplev, at_eof); diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index f569d4045ec..1dddbaab38b 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -3360,7 +3360,9 @@ import_export_decl (tree decl) * implicit instantiations of function templates - * inline function + * inline functions + + * inline variables * implicit instantiations of static data members of class templates @@ -3383,6 +3385,7 @@ import_export_decl (tree decl) || DECL_DECLARED_INLINE_P (decl)); else gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl) + || DECL_INLINE_VAR_P (decl) || DECL_VTABLE_OR_VTT_P (decl) || DECL_TINFO_P (decl)); /* Check that a definition of DECL is available in this translation @@ -3511,7 +3514,7 @@ import_export_decl (tree decl) this entity as undefined in this translation unit. */ import_p = true; } - else if (DECL_FUNCTION_MEMBER_P (decl)) + else if (TREE_CODE (decl) == FUNCTION_DECL && DECL_FUNCTION_MEMBER_P (decl)) { if (!DECL_DECLARED_INLINE_P (decl)) { diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C index 85f74a91521..7ec20afc065 100644 --- a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C +++ b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C @@ -8,6 +8,8 @@ // { dg-final { scan-assembler-times " DW_AT_\[^\n\r]*linkage_name" 7 } } inline int a; +int& ar = a; + struct S { static inline double b = 4.0; diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C index 6345b5e2ec8..bb40a229551 100644 --- a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C +++ b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C @@ -11,6 +11,8 @@ // { dg-final { scan-assembler-times " DW_AT_\[^\n\r]*linkage_name" 7 } } inline int a; +int& ar = a; + struct S { static inline double b = 4.0; diff --git a/gcc/testsuite/g++.dg/modules/init-7_a.H b/gcc/testsuite/g++.dg/modules/init-7_a.H new file mode 100644 index 00000000000..7a0bb721c30 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-7_a.H @@ -0,0 +1,6 @@ +// PR c++/113708 +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +inline int f() { return 42; } +inline int a = f(); diff --git a/gcc/testsuite/g++.dg/modules/init-7_b.C b/gcc/testsuite/g++.dg/modules/init-7_b.C new file mode 100644 index 00000000000..58bb0620ca5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-7_b.C @@ -0,0 +1,6 @@ +// PR c++/113708 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import "init-7_a.H"; +int main() { a; }
On 2/14/24 06:03, Nathaniel Shead wrote: > On Tue, Feb 13, 2024 at 09:47:27PM -0500, Jason Merrill wrote: >> On 2/13/24 20:34, Nathaniel Shead wrote: >>> On Tue, Feb 13, 2024 at 06:08:42PM -0500, Jason Merrill wrote: >>>> On 2/11/24 08:26, Nathaniel Shead wrote: >>>>> >>>>> Currently inline vars imported from modules aren't correctly finalised, >>>>> which means that import_export_decl gets called at the end of TU >>>>> processing despite not being meant to for these kinds of declarations. >>>> >>>> I disagree that it's not meant to; inline variables are vague linkage just >>>> like template instantiations, so the bug seems to be that import_export_decl >>>> doesn't accept them. And on the other side, that make_rtl_for_nonlocal_decl >>>> doesn't defer them like instantations. >>>> >>>> Jason >>>> >>> >>> True, that's a good point. I think I confused myself here. >>> >>> Here's a fixed patch that looks a lot cleaner. Bootstrapped and >>> regtested (so far just dg.exp and modules.exp) on x86_64-pc-linux-gnu, >>> OK for trunk if full regtest succeeds? >> >> OK. >> > > A full bootstrap failed two tests in dwarf2.exp, which seem to be caused > by an unreferenced 'inline' variable not being emitted into the debug > info and thus causing the checks for its existence to fail. Adding a > reference to the vars cause the tests to pass. > > Now fully bootstrapped and regtested on x86_64-pc-linux-gnu, still OK > for trunk? (Only change is the two adjusted testcases.) OK. > -- >8 -- > > Inline variables are vague-linkage, and may or may not need to be > emitted in any TU that they are part of, similarly to e.g. template > instantiations. > > Currently 'import_export_decl' assumes that inline variables have > already been emitted when it comes to end-of-TU processing, and so > crashes when importing non-trivially-initialised variables from a > module, as they have not yet been finalised. > > This patch fixes this by ensuring that inline variables are always > deferred till end-of-TU processing, unifying the behaviour for module > and non-module code. > > PR c++/113708 > > gcc/cp/ChangeLog: > > * decl.cc (make_rtl_for_nonlocal_decl): Defer inline variables. > * decl2.cc (import_export_decl): Support inline variables. > > gcc/testsuite/ChangeLog: > > * g++.dg/debug/dwarf2/inline-var-1.C: Reference 'a' to ensure it > is emitted. > * g++.dg/debug/dwarf2/inline-var-3.C: Likewise. > * g++.dg/modules/init-7_a.H: New test. > * g++.dg/modules/init-7_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/decl.cc | 4 ++++ > gcc/cp/decl2.cc | 7 +++++-- > gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C | 2 ++ > gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C | 2 ++ > gcc/testsuite/g++.dg/modules/init-7_a.H | 6 ++++++ > gcc/testsuite/g++.dg/modules/init-7_b.C | 6 ++++++ > 6 files changed, 25 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/init-7_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/init-7_b.C > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 3e41fd4fa31..969513c069a 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -7954,6 +7954,10 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec) > && DECL_IMPLICIT_INSTANTIATION (decl)) > defer_p = 1; > > + /* Defer vague-linkage variables. */ > + if (DECL_INLINE_VAR_P (decl)) > + defer_p = 1; > + > /* If we're not deferring, go ahead and assemble the variable. */ > if (!defer_p) > rest_of_decl_compilation (decl, toplev, at_eof); > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index f569d4045ec..1dddbaab38b 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -3360,7 +3360,9 @@ import_export_decl (tree decl) > > * implicit instantiations of function templates > > - * inline function > + * inline functions > + > + * inline variables > > * implicit instantiations of static data members of class > templates > @@ -3383,6 +3385,7 @@ import_export_decl (tree decl) > || DECL_DECLARED_INLINE_P (decl)); > else > gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl) > + || DECL_INLINE_VAR_P (decl) > || DECL_VTABLE_OR_VTT_P (decl) > || DECL_TINFO_P (decl)); > /* Check that a definition of DECL is available in this translation > @@ -3511,7 +3514,7 @@ import_export_decl (tree decl) > this entity as undefined in this translation unit. */ > import_p = true; > } > - else if (DECL_FUNCTION_MEMBER_P (decl)) > + else if (TREE_CODE (decl) == FUNCTION_DECL && DECL_FUNCTION_MEMBER_P (decl)) > { > if (!DECL_DECLARED_INLINE_P (decl)) > { > diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C > index 85f74a91521..7ec20afc065 100644 > --- a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C > +++ b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-1.C > @@ -8,6 +8,8 @@ > // { dg-final { scan-assembler-times " DW_AT_\[^\n\r]*linkage_name" 7 } } > > inline int a; > +int& ar = a; > + > struct S > { > static inline double b = 4.0; > diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C > index 6345b5e2ec8..bb40a229551 100644 > --- a/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C > +++ b/gcc/testsuite/g++.dg/debug/dwarf2/inline-var-3.C > @@ -11,6 +11,8 @@ > // { dg-final { scan-assembler-times " DW_AT_\[^\n\r]*linkage_name" 7 } } > > inline int a; > +int& ar = a; > + > struct S > { > static inline double b = 4.0; > diff --git a/gcc/testsuite/g++.dg/modules/init-7_a.H b/gcc/testsuite/g++.dg/modules/init-7_a.H > new file mode 100644 > index 00000000000..7a0bb721c30 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/init-7_a.H > @@ -0,0 +1,6 @@ > +// PR c++/113708 > +// { dg-additional-options "-fmodule-header" } > +// { dg-module-cmi {} } > + > +inline int f() { return 42; } > +inline int a = f(); > diff --git a/gcc/testsuite/g++.dg/modules/init-7_b.C b/gcc/testsuite/g++.dg/modules/init-7_b.C > new file mode 100644 > index 00000000000..58bb0620ca5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/init-7_b.C > @@ -0,0 +1,6 @@ > +// PR c++/113708 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts" } > + > +import "init-7_a.H"; > +int main() { a; }
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index 3e41fd4fa31..969513c069a 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -7954,6 +7954,10 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec) && DECL_IMPLICIT_INSTANTIATION (decl)) defer_p = 1; + /* Defer vague-linkage variables. */ + if (DECL_INLINE_VAR_P (decl)) + defer_p = 1; + /* If we're not deferring, go ahead and assemble the variable. */ if (!defer_p) rest_of_decl_compilation (decl, toplev, at_eof); diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index f569d4045ec..1dddbaab38b 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -3360,7 +3360,9 @@ import_export_decl (tree decl) * implicit instantiations of function templates - * inline function + * inline functions + + * inline variables * implicit instantiations of static data members of class templates @@ -3383,6 +3385,7 @@ import_export_decl (tree decl) || DECL_DECLARED_INLINE_P (decl)); else gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl) + || DECL_INLINE_VAR_P (decl) || DECL_VTABLE_OR_VTT_P (decl) || DECL_TINFO_P (decl)); /* Check that a definition of DECL is available in this translation @@ -3511,7 +3514,7 @@ import_export_decl (tree decl) this entity as undefined in this translation unit. */ import_p = true; } - else if (DECL_FUNCTION_MEMBER_P (decl)) + else if (TREE_CODE (decl) == FUNCTION_DECL && DECL_FUNCTION_MEMBER_P (decl)) { if (!DECL_DECLARED_INLINE_P (decl)) { diff --git a/gcc/testsuite/g++.dg/modules/init-7_a.H b/gcc/testsuite/g++.dg/modules/init-7_a.H new file mode 100644 index 00000000000..7a0bb721c30 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-7_a.H @@ -0,0 +1,6 @@ +// PR c++/113708 +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +inline int f() { return 42; } +inline int a = f(); diff --git a/gcc/testsuite/g++.dg/modules/init-7_b.C b/gcc/testsuite/g++.dg/modules/init-7_b.C new file mode 100644 index 00000000000..58bb0620ca5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-7_b.C @@ -0,0 +1,6 @@ +// PR c++/113708 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import "init-7_a.H"; +int main() { a; }