| Message ID | 20240306191042.1883372-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: inline namespace abi_tag streaming [PR110730] | expand |
On 3/6/24 14:10, Patrick Palka wrote: > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > > -- >8 -- > > The unreduced testcase from this PR crashes at runtime ultimately > because we don't stream the abi_tag attribute on inline namespaces and > so the filesystem::current_path() call resolves to the non-C++11 ABI > version even though the C++11 ABI is active, leading to a crash when > destructing the call result (which contains an std::string member). > > While we do stream the DECL_ATTRIBUTES of all decls that go through > the generic tree streaming routines, it seems namespaces are streamed > separately from other decls and we don't use the generic routines for > them. So this patch makes us stream the abi_tag manually for (inline) > namespaces. Why not stream all DECL_ATTRIBUTES of all namespaces? Jason
On Wed, 6 Mar 2024, Jason Merrill wrote: > On 3/6/24 14:10, Patrick Palka wrote: > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > > > > -- >8 -- > > > > The unreduced testcase from this PR crashes at runtime ultimately > > because we don't stream the abi_tag attribute on inline namespaces and > > so the filesystem::current_path() call resolves to the non-C++11 ABI > > version even though the C++11 ABI is active, leading to a crash when > > destructing the call result (which contains an std::string member). > > > > While we do stream the DECL_ATTRIBUTES of all decls that go through > > the generic tree streaming routines, it seems namespaces are streamed > > separately from other decls and we don't use the generic routines for > > them. So this patch makes us stream the abi_tag manually for (inline) > > namespaces. > > Why not stream all DECL_ATTRIBUTES of all namespaces? AFAICT abi_tag and deprecated are the only attributes that we recognize on a namespace, and for deprecated it should suffice to stream the TREE_DEPRECATED flag instead of the actual attribute, so hardcoding abi_tag streaming seems convenient. If we wanted to stream all DECL_ATTRIBUTES of a namespace then we'd have to assume up front what kind of tree arguments of the attributes can be, e.g. an INTEGER_CST or a STRING_CST etc and implement streaming of these trees within the bytes_in/out base classes instead of trees_in/out (we only have access to a bytes_in/out object from read/write_namespaces). I'm not sure that's worth it unless there's other namespace attributes we want to stream? > > Jason > >
On 3/6/24 21:12, Patrick Palka wrote: > On Wed, 6 Mar 2024, Jason Merrill wrote: > >> On 3/6/24 14:10, Patrick Palka wrote: >>> Tested on x86_64-pc-linux-gnu, does this look OK for trunk? >>> >>> -- >8 -- >>> >>> The unreduced testcase from this PR crashes at runtime ultimately >>> because we don't stream the abi_tag attribute on inline namespaces and >>> so the filesystem::current_path() call resolves to the non-C++11 ABI >>> version even though the C++11 ABI is active, leading to a crash when >>> destructing the call result (which contains an std::string member). >>> >>> While we do stream the DECL_ATTRIBUTES of all decls that go through >>> the generic tree streaming routines, it seems namespaces are streamed >>> separately from other decls and we don't use the generic routines for >>> them. So this patch makes us stream the abi_tag manually for (inline) >>> namespaces. >> >> Why not stream all DECL_ATTRIBUTES of all namespaces? > > AFAICT abi_tag and deprecated are the only attributes that we recognize > on a namespace, and for deprecated it should suffice to stream the > TREE_DEPRECATED flag instead of the actual attribute, so hardcoding > abi_tag streaming seems convenient. > > If we wanted to stream all DECL_ATTRIBUTES of a namespace then we'd have > to assume up front what kind of tree arguments of the attributes can be, > e.g. an INTEGER_CST or a STRING_CST etc and implement streaming of these > trees within the bytes_in/out base classes instead of trees_in/out (we > only have access to a bytes_in/out object from read/write_namespaces). Hunh, why don't we use trees_in/out for namespaces? But in that case, the patch is OK. You might use list_length (tags) instead of writing the counting loop yourself. OK either way. Jason
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index d9e34e9a4b9..ce62c3341a6 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -15411,6 +15411,22 @@ module_state::write_namespaces (elf_out *to, vec<depset *> spaces, sec.u (flags); write_location (sec, DECL_SOURCE_LOCATION (ns)); + + if (DECL_NAMESPACE_INLINE_P (ns)) + { + if (tree tags = lookup_attribute ("abi_tag", DECL_ATTRIBUTES (ns))) + { + tags = TREE_VALUE (tags); + unsigned count = 0; + for (tree tag = tags; tag; tag = TREE_CHAIN (tag)) + ++count; + sec.u (count); + for (tree tag = tags; tag; tag = TREE_CHAIN (tag)) + sec.str (TREE_STRING_POINTER (TREE_VALUE (tag))); + } + else + sec.u (0); + } } sec.end (to, to->name (MOD_SNAME_PFX ".nms"), crc_p); @@ -15441,6 +15457,7 @@ module_state::read_namespaces (unsigned num) /* See comment in write_namespace about why not bits. */ unsigned flags = sec.u (); location_t src_loc = read_location (sec); + unsigned tags_count = (flags & 2) ? sec.u () : 0; if (entity_index >= entity_num || !parent @@ -15449,6 +15466,17 @@ module_state::read_namespaces (unsigned num) if (sec.get_overrun ()) break; + tree tags = NULL_TREE; + while (tags_count--) + { + size_t len; + const char *str = sec.str (&len); + if (sec.get_overrun ()) + break; + tags = tree_cons (NULL_TREE, build_string (len + 1, str), tags); + tags = nreverse (tags); + } + tree id = name ? get_identifier (from ()->name (name)) : NULL_TREE; dump () && dump ("Read namespace:%u %P%s%s%s%s", @@ -15477,6 +15505,10 @@ module_state::read_namespaces (unsigned num) DECL_MODULE_EXPORT_P (inner) = true; } + if (tags) + DECL_ATTRIBUTES (inner) + = tree_cons (get_identifier ("abi_tag"), tags, DECL_ATTRIBUTES (inner)); + /* Install the namespace. */ (*entity_ary)[entity_lwm + entity_index] = inner; if (DECL_MODULE_IMPORT_P (inner)) diff --git a/gcc/testsuite/g++.dg/modules/namespace-6_a.H b/gcc/testsuite/g++.dg/modules/namespace-6_a.H new file mode 100644 index 00000000000..b412cbe6cbf --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/namespace-6_a.H @@ -0,0 +1,10 @@ +// PR c++/110730 +// { dg-additional-options -fmodule-header } +// { dg-module-cmi {} } + +namespace std::filesystem { + inline namespace __cxx11 __attribute__((__abi_tag__("cxx11", "foo"))) { + struct path { }; + } + inline path current_path() { return {}; }; +} diff --git a/gcc/testsuite/g++.dg/modules/namespace-6_b.C b/gcc/testsuite/g++.dg/modules/namespace-6_b.C new file mode 100644 index 00000000000..2ce41c7c9c7 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/namespace-6_b.C @@ -0,0 +1,7 @@ +// PR c++/110730 +// { dg-additional-options -fmodules-ts } + +import "namespace-6_a.H"; + +int main() { std::filesystem::current_path(); } +// { dg-final { scan-assembler _ZNSt10filesystem12current_pathB5cxx11B3fooEv } }