[PATCH] c++/modules: inline namespace abi_tag streaming [PR110730]

[PATCH] c++/modules: inline namespace abi_tag streaming [PR110730]

[Patrick Palka]

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.

	PR c++/110730

gcc/cp/ChangeLog:

	* module.cc (module_state::write_namespaces): Stream the
	abi_tag attribute of an inline namespace.
	(module_state::read_namespaces): Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/namespace-6_a.H: New test.
	* g++.dg/modules/namespace-6_b.C: New test.
---
 gcc/cp/module.cc                             | 32 ++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/namespace-6_a.H | 10 ++++++
 gcc/testsuite/g++.dg/modules/namespace-6_b.C |  7 +++++
 3 files changed, 49 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/namespace-6_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/namespace-6_b.C

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 } }
-- 
2.44.0.117.g43072b4ca1

Re: [PATCH] c++/modules: inline namespace abi_tag streaming [PR110730]

[Jason Merrill]

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

Re: [PATCH] c++/modules: inline namespace abi_tag streaming [PR110730]

[Patrick Palka]

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
> 
> 

Re: [PATCH] c++/modules: inline namespace abi_tag streaming [PR110730]

[Jason Merrill]

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