Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On Wed, 3 Jan 2024, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> Static data members marked 'inline' should be emitted in TUs where they
> are ODR-used. We need to make sure that statics imported from modules
> are correctly added to the 'pending_statics' map so that they get
> emitted if needed, otherwise the attached testcase fails to link.
>
> PR c++/112899
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (note_variable_template_instantiation): Rename to...
> (note_static_storage_variable): ...this.
> * decl2.cc (note_variable_template_instantiation): Rename to...
> (note_static_storage_variable): ...this.
> * pt.cc (instantiate_decl): Rename usage of above function.
> * module.cc (trees_in::read_var_def): Remember pending statics
> that we stream in.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/init-4_a.C: New test.
> * g++.dg/modules/init-4_b.C: New test.
>
> Signed-off-by: Nathaniel Shead
> ---
> gcc/cp/cp-tree.h| 2 +-
> gcc/cp/decl2.cc | 4 ++--
> gcc/cp/module.cc| 4
> gcc/cp/pt.cc| 2 +-
> gcc/testsuite/g++.dg/modules/init-4_a.C | 9 +
> gcc/testsuite/g++.dg/modules/init-4_b.C | 11 +++
> 6 files changed, 28 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/init-4_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/init-4_b.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 1979572c365..ebd2850599a 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7113,7 +7113,7 @@ extern tree maybe_get_tls_wrapper_call (tree);
> extern void mark_needed (tree);
> extern bool decl_needed_p(tree);
> extern void note_vague_linkage_fn(tree);
> -extern void note_variable_template_instantiation (tree);
> +extern void note_static_storage_variable (tree);
> extern tree build_artificial_parm(tree, tree, tree);
> extern bool possibly_inlined_p (tree);
> extern int parm_index (tree);
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 0850d3f5bce..241216b0dfe 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -910,10 +910,10 @@ note_vague_linkage_fn (tree decl)
>vec_safe_push (deferred_fns, decl);
> }
>
> -/* As above, but for variable template instantiations. */
> +/* As above, but for variables with static storage duration. */
>
> void
> -note_variable_template_instantiation (tree decl)
> +note_static_storage_variable (tree decl)
> {
>vec_safe_push (pending_statics, decl);
> }
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 0bd46414da9..14818131a70 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -11752,6 +11752,10 @@ trees_in::read_var_def (tree decl, tree
> maybe_template)
> DECL_INITIALIZED_P (decl) = true;
> if (maybe_dup && DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P
> (maybe_dup))
> DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = true;
> + if (DECL_CONTEXT (decl)
> + && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (decl))
> + && !DECL_TEMPLATE_INFO (decl))
> + note_static_storage_variable (decl);
It seems this should also handle templated inlines via
&& (!DECL_TEMPLATE_INFO (decl)
|| DECL_IMPLICIT_INSTANTIATION (decl))
otherwise the following fails to link:
$ cat init-5_a.H
template
struct __from_chars_alnum_to_val_table {
static inline int value = 42;
};
inline unsigned char
__from_chars_alnum_to_val() {
return __from_chars_alnum_to_val_table::value;
}
$ cat init-6_b.C
import "init-5_a.H";
int main() {
__from_chars_alnum_to_val();
}
$ g++ -fmodules-ts -std=c++20 init-5_a.H init-5_b.C
/usr/bin/ld: /tmp/ccNRaads.o: in function `__from_chars_alnum_to_val()':
init-6_b.C:(.text._Z25__from_chars_alnum_to_valv[_Z25__from_chars_alnum_to_valv]+0x6):
undefined reference to `__from_chars_alnum_to_val_table::value'
By the way I ran into this when testing out std::print with modules:
$ cat std.C
export module std;
export import ;
$ cat hello.C
import std;
int main() {
std::print("Hello {}!", "World");
}
$ g++ -fmodules-ts -std=c++26 -x c++-system-header bits/stdc++.h
$ g++ -fmodules-ts -std=c++26 std.C hello.C && ./a.out # before
/usr/bin/ld: /tmp/ccqNgOM1.o: in function `unsigned char
std::__detail::__from_chars_alnum_to_val(unsigned char)':
hello.C:(.text._ZNSt8__detail25__from_chars_alnum_to_valILb0EEEhh[_ZNSt8__detail25__from_chars_alnum_to_valILb0EEEhh]+0x12):
undefined reference to
`std::__detail::__from_chars_alnum_to_val_table::value'
$ g++ -fmodules-ts -std=c++26 std.C hello.C && ./a.out # after
Hello World!
It's great that this is so close to working!
> }
>
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On 1/8/24 04:21, Iain Sandoe wrote: On 6 Jan 2024, at 22:30, Nathan Sidwell wrote: Richard Smith & I discussed whether we should use the module interface's capability of giving vague linkage entities a strong location. I didn't want to go messing with that, 'cos it was changing yet more stuff. But, perhaps we should revisit that? Any keyless polymorphic class in module purview gets its vtables etc emitted in the module's object file? Likewise these kinds of entities. cc'ing Iain, who probably knows more about Clang's state here. I have been trying to keep up with this thread, but not sure if I can throw a whole lot of light on things. There is an on-going attempt (now some 3 or 4 papers in) to try and figure out how to handle `static inline` entities at least at file scope - but that appears to be a different case (I can try an locate the latest paper on this if needed; the topic was discussed in Varna and Kona, but no new paper yet - perhaps Michael [Spencer] will bring a paper in Tokyo). clang ran into some issues with vtables and that resulted in some discussion about whether there should be an amendment to the Itanium ABI to deal with the module-specific stuff. https://github.com/itanium-cxx-abi/cxx-abi/issues/170 https://github.com/llvm/llvm-project/pull/75912#discussion_r1444150069 Sorry I cannot be much more specific at present, That's pretty specific that vtables at least get emitted in the module whether or not there's a key function. I've asked on that issue why this only applies to vtables. Jason
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
> On 6 Jan 2024, at 22:30, Nathan Sidwell wrote: > > Richard Smith & I discussed whether we should use the module interface's > capability of giving vague linkage entities a strong location. I didn't want > to go messing with that, 'cos it was changing yet more stuff. > > But, perhaps we should revisit that? Any keyless polymorphic class in module > purview gets its vtables etc emitted in the module's object file? Likewise > these kinds of entities. > > cc'ing Iain, who probably knows more about Clang's state here. I have been trying to keep up with this thread, but not sure if I can throw a whole lot of light on things. There is an on-going attempt (now some 3 or 4 papers in) to try and figure out how to handle `static inline` entities at least at file scope - but that appears to be a different case (I can try an locate the latest paper on this if needed; the topic was discussed in Varna and Kona, but no new paper yet - perhaps Michael [Spencer] will bring a paper in Tokyo). clang ran into some issues with vtables and that resulted in some discussion about whether there should be an amendment to the Itanium ABI to deal with the module-specific stuff. https://github.com/itanium-cxx-abi/cxx-abi/issues/170 https://github.com/llvm/llvm-project/pull/75912#discussion_r1444150069 Sorry I cannot be much more specific at present, Iain > > nathan > > > On 1/4/24 21:06, Jason Merrill wrote: >> On 1/4/24 18:02, Nathaniel Shead wrote: >>> On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote: On 1/4/24 17:24, Nathaniel Shead wrote: > On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote: >> On 1/2/24 17:40, Nathaniel Shead wrote: >>> Static data members marked 'inline' should be emitted in TUs where they >>> are ODR-used. We need to make sure that statics imported from modules >>> are correctly added to the 'pending_statics' map so that they get >>> emitted if needed, otherwise the attached testcase fails to link. >> >> Hmm, this seems wrong to me; I'd think that static data members marked >> inline should be emitted in the module, and not in importers. > > That's what I'd initially thought too, but this is at least consistent > with non-class inlines (variables and functions), which similarly only > get emitted in TUs that they're ODR-used rather than always (and only) > being emitted within the module. > > I guess an alternative would be to change it around so that all > exported definitions are marked as needed in the module interface file > (and emitted there), and then setting some flag so that they're never > emitted in importers. Yes, that would be my expectation. What do other modules implementations do? >>> >>> Clang only emits ODR-used declarations (same as GCC currently). >>> >>> MSVC emits all inline variables (whether exported or not) but no inline >>> functions. >> Hmm, not a strong vote for my direction. > I'm not entirely sure what flag that would be > though, I still haven't quite wrapped my head what controls what with > regards to this, and I'm not convinced it wouldn't break template > instantiations. I would guess avoid emitting if DECL_MODULE_IMPORT_P && DECL_MODULE_ATTACH_P. >>> >>> Ah yup, that would make sense. I guess, thinking about it more, we >>> should then also ensure that all TREE_PUBLIC declarations are emitted in >>> the module interface even if not exported, since they may be needed in >>> implementation units? >> That would also make sense to me; since we know the module interface unit is >> compiled to an object file, everything vague linkage in it can go there. > I wonder if this might also be related to the issue Nathan noted with > regards to block-scope class methods, which I haven't completely worked > out how to solve yet otherwise (see > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html). Indeed. >>> >>> I'll give implementing this a try then, if you think that would be >>> sensible. (Where by "this" I mean "emit all public declarations in >>> module interface files, whether used or not".) >> I'd like to hear Nathan's thoughts on the matter first, since he's the >> modules implementation designer. >> Jason > > -- > Nathan Sidwell >
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
Richard Smith & I discussed whether we should use the module interface's capability of giving vague linkage entities a strong location. I didn't want to go messing with that, 'cos it was changing yet more stuff. But, perhaps we should revisit that? Any keyless polymorphic class in module purview gets its vtables etc emitted in the module's object file? Likewise these kinds of entities. cc'ing Iain, who probably knows more about Clang's state here. nathan On 1/4/24 21:06, Jason Merrill wrote: On 1/4/24 18:02, Nathaniel Shead wrote: On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote: On 1/4/24 17:24, Nathaniel Shead wrote: On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote: On 1/2/24 17:40, Nathaniel Shead wrote: Static data members marked 'inline' should be emitted in TUs where they are ODR-used. We need to make sure that statics imported from modules are correctly added to the 'pending_statics' map so that they get emitted if needed, otherwise the attached testcase fails to link. Hmm, this seems wrong to me; I'd think that static data members marked inline should be emitted in the module, and not in importers. That's what I'd initially thought too, but this is at least consistent with non-class inlines (variables and functions), which similarly only get emitted in TUs that they're ODR-used rather than always (and only) being emitted within the module. I guess an alternative would be to change it around so that all exported definitions are marked as needed in the module interface file (and emitted there), and then setting some flag so that they're never emitted in importers. Yes, that would be my expectation. What do other modules implementations do? Clang only emits ODR-used declarations (same as GCC currently). MSVC emits all inline variables (whether exported or not) but no inline functions. Hmm, not a strong vote for my direction. I'm not entirely sure what flag that would be though, I still haven't quite wrapped my head what controls what with regards to this, and I'm not convinced it wouldn't break template instantiations. I would guess avoid emitting if DECL_MODULE_IMPORT_P && DECL_MODULE_ATTACH_P. Ah yup, that would make sense. I guess, thinking about it more, we should then also ensure that all TREE_PUBLIC declarations are emitted in the module interface even if not exported, since they may be needed in implementation units? That would also make sense to me; since we know the module interface unit is compiled to an object file, everything vague linkage in it can go there. I wonder if this might also be related to the issue Nathan noted with regards to block-scope class methods, which I haven't completely worked out how to solve yet otherwise (see https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html). Indeed. I'll give implementing this a try then, if you think that would be sensible. (Where by "this" I mean "emit all public declarations in module interface files, whether used or not".) I'd like to hear Nathan's thoughts on the matter first, since he's the modules implementation designer. Jason -- Nathan Sidwell
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On 1/4/24 18:02, Nathaniel Shead wrote: On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote: On 1/4/24 17:24, Nathaniel Shead wrote: On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote: On 1/2/24 17:40, Nathaniel Shead wrote: Static data members marked 'inline' should be emitted in TUs where they are ODR-used. We need to make sure that statics imported from modules are correctly added to the 'pending_statics' map so that they get emitted if needed, otherwise the attached testcase fails to link. Hmm, this seems wrong to me; I'd think that static data members marked inline should be emitted in the module, and not in importers. That's what I'd initially thought too, but this is at least consistent with non-class inlines (variables and functions), which similarly only get emitted in TUs that they're ODR-used rather than always (and only) being emitted within the module. I guess an alternative would be to change it around so that all exported definitions are marked as needed in the module interface file (and emitted there), and then setting some flag so that they're never emitted in importers. Yes, that would be my expectation. What do other modules implementations do? Clang only emits ODR-used declarations (same as GCC currently). MSVC emits all inline variables (whether exported or not) but no inline functions. Hmm, not a strong vote for my direction. I'm not entirely sure what flag that would be though, I still haven't quite wrapped my head what controls what with regards to this, and I'm not convinced it wouldn't break template instantiations. I would guess avoid emitting if DECL_MODULE_IMPORT_P && DECL_MODULE_ATTACH_P. Ah yup, that would make sense. I guess, thinking about it more, we should then also ensure that all TREE_PUBLIC declarations are emitted in the module interface even if not exported, since they may be needed in implementation units? That would also make sense to me; since we know the module interface unit is compiled to an object file, everything vague linkage in it can go there. I wonder if this might also be related to the issue Nathan noted with regards to block-scope class methods, which I haven't completely worked out how to solve yet otherwise (see https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html). Indeed. I'll give implementing this a try then, if you think that would be sensible. (Where by "this" I mean "emit all public declarations in module interface files, whether used or not".) I'd like to hear Nathan's thoughts on the matter first, since he's the modules implementation designer. Jason
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote: > On 1/4/24 17:24, Nathaniel Shead wrote: > > On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote: > > > On 1/2/24 17:40, Nathaniel Shead wrote: > > > > Static data members marked 'inline' should be emitted in TUs where they > > > > are ODR-used. We need to make sure that statics imported from modules > > > > are correctly added to the 'pending_statics' map so that they get > > > > emitted if needed, otherwise the attached testcase fails to link. > > > > > > Hmm, this seems wrong to me; I'd think that static data members marked > > > inline should be emitted in the module, and not in importers. > > > > That's what I'd initially thought too, but this is at least consistent > > with non-class inlines (variables and functions), which similarly only > > get emitted in TUs that they're ODR-used rather than always (and only) > > being emitted within the module. > > > > I guess an alternative would be to change it around so that all > > exported definitions are marked as needed in the module interface file > > (and emitted there), and then setting some flag so that they're never > > emitted in importers. > > Yes, that would be my expectation. What do other modules implementations > do? Clang only emits ODR-used declarations (same as GCC currently). MSVC emits all inline variables (whether exported or not) but no inline functions. > > I'm not entirely sure what flag that would be > > though, I still haven't quite wrapped my head what controls what with > > regards to this, and I'm not convinced it wouldn't break template > > instantiations. > > I would guess avoid emitting if DECL_MODULE_IMPORT_P && > DECL_MODULE_ATTACH_P. Ah yup, that would make sense. I guess, thinking about it more, we should then also ensure that all TREE_PUBLIC declarations are emitted in the module interface even if not exported, since they may be needed in implementation units? > > I wonder if this might also be related to the issue Nathan noted with > > regards to block-scope class methods, which I haven't completely worked > > out how to solve yet otherwise (see > > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html). > > Indeed. > > Jason > I'll give implementing this a try then, if you think that would be sensible. (Where by "this" I mean "emit all public declarations in module interface files, whether used or not".)
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On 1/4/24 17:24, Nathaniel Shead wrote: On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote: On 1/2/24 17:40, Nathaniel Shead wrote: Static data members marked 'inline' should be emitted in TUs where they are ODR-used. We need to make sure that statics imported from modules are correctly added to the 'pending_statics' map so that they get emitted if needed, otherwise the attached testcase fails to link. Hmm, this seems wrong to me; I'd think that static data members marked inline should be emitted in the module, and not in importers. That's what I'd initially thought too, but this is at least consistent with non-class inlines (variables and functions), which similarly only get emitted in TUs that they're ODR-used rather than always (and only) being emitted within the module. I guess an alternative would be to change it around so that all exported definitions are marked as needed in the module interface file (and emitted there), and then setting some flag so that they're never emitted in importers. Yes, that would be my expectation. What do other modules implementations do? I'm not entirely sure what flag that would be though, I still haven't quite wrapped my head what controls what with regards to this, and I'm not convinced it wouldn't break template instantiations. I would guess avoid emitting if DECL_MODULE_IMPORT_P && DECL_MODULE_ATTACH_P. I wonder if this might also be related to the issue Nathan noted with regards to block-scope class methods, which I haven't completely worked out how to solve yet otherwise (see https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html). Indeed. Jason
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote: > On 1/2/24 17:40, Nathaniel Shead wrote: > > Static data members marked 'inline' should be emitted in TUs where they > > are ODR-used. We need to make sure that statics imported from modules > > are correctly added to the 'pending_statics' map so that they get > > emitted if needed, otherwise the attached testcase fails to link. > > Hmm, this seems wrong to me; I'd think that static data members marked > inline should be emitted in the module, and not in importers. > > Jason > That's what I'd initially thought too, but this is at least consistent with non-class inlines (variables and functions), which similarly only get emitted in TUs that they're ODR-used rather than always (and only) being emitted within the module. I guess an alternative would be to change it around so that all exported definitions are marked as needed in the module interface file (and emitted there), and then setting some flag so that they're never emitted in importers. I'm not entirely sure what flag that would be though, I still haven't quite wrapped my head what controls what with regards to this, and I'm not convinced it wouldn't break template instantiations. I wonder if this might also be related to the issue Nathan noted with regards to block-scope class methods, which I haven't completely worked out how to solve yet otherwise (see https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html). Nathaniel
Re: c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]
On 1/2/24 17:40, Nathaniel Shead wrote: Static data members marked 'inline' should be emitted in TUs where they are ODR-used. We need to make sure that statics imported from modules are correctly added to the 'pending_statics' map so that they get emitted if needed, otherwise the attached testcase fails to link. Hmm, this seems wrong to me; I'd think that static data members marked inline should be emitted in the module, and not in importers. Jason
