Re: [PATCH] c++: parser - Support for target address spaces in C++

[Paul Iannetta <piannetta@kalrayinc.com>]

Hi,

On Thu, Oct 06, 2022 at 01:34:40PM -0400, Jason Merrill wrote:
[snip]
> 
> Hmm?  We mangle __restrict:
> 
> void f(int *__restrict *p) { } // _Z1fPrPi
> 

Indeed, I have overlooked that point.  Thank you for pointing it out.

> but cv-qualifiers (including restrict) are dropped on the top-level type of
> a parameter.
> 
> If address spaces should be mangled, it probably makes sense to use the same
> <extended-qualifier> mangling that we already use for attributes in
> write_CV_qualifiers_for_type.
> 

That's a nice feature! LLVM is using AS<number> to mangle the
address-name qualified types but that would have required an update of
libiberty's demangler in the binutils as well.  Following your advice,
I implemented the mangling of address_names as U<len>__address_name
(eg., U8__seg_fs), underscores may appear but that does not seem to be
a problem.

I don't think that there should be anything to do on the template
side, but I am no expert of what is expected of templates when it
comes to strip or forward qualifiers. So, please tell me if I
overlooked something on that side.

Now concerning the meat of the patch itself.

The C part of the support of address spaces does not appear to have
dedicated tests, especially since it is mostly a target specific
feature.  I, however, added a few tests, one for the mangling,
and others which test the different error messages that might
appear when using address spaces in an unsafe way.

Most of the patch tries to mirror the commit
               309434d678721acedb299f20cdc2b4778062b180
which introduces address spaces to the C front-end.  The C and C++
parsers share a lot but are subtly different so there might be some
discrepancies.  In particular, I ensure that address spaces do not
conflict, that only one address space can be specified at a time, and
that the type conversion mechanism pick the superset of the address
spaces if available. I also forbid address spaces in compound literals.

I have only tested the patch on x86 and on our not-yet-upstream
architecture, without seeing any regressions.

NB: I do not add a DSO for the moment because I have started the process of
assigning copyright to the FSF.

# ------------------------ >8 ------------------------
Add support for custom address spaces in C++

gcc/
        * tree.h (ENCODE_QUAL_ADDR_SPACE): Missing parentheses.

gcc/c/
        * c-decl.cc: Remove c_register_addr_space.

gcc/c-family/
        * c-common.cc (c_register_addr_space): Imported from c-decl.cc
        (addr_space_superset): Imported from gcc/c/c-typecheck.cc
        * c-common.h: Remove the FIXME.
        (addr_space_superset): Add prototype.

gcc/cp/
        * cp-tree.h (enum cp_decl_spec): Add addr_space support.
        (struct cp_decl_specifier_seq): Likewise.
        * decl.cc (get_type_quals): Likewise.
        (check_tag_decl): Likewise.
        * parser.cc (cp_parser_type_specifier): Likewise.
        (cp_parser_cv_qualifier_seq_opt): Likewise.
        (cp_parser_postfix_expression): Likewise.
        (cp_parser_type_specifier): Likewise.
        (set_and_check_decl_spec_loc): Likewise.
        * typeck.cc (composite_pointer_type): Likewise
        (comp_ptr_ttypes_real): Likewise.
        * tree.cc: Remove c_register_addr_space stub.
        * mangle.cc (write_CV_qualifiers_for_type): Mangle address spaces
          using the extended qualifier notation.

gcc/doc
        * extend.texi (Named Address Spaces): add a mention about C++
          support.

gcc/testsuite/
        * g++.dg/abi/mangle-addr-space1.C: New test.
        * g++.dg/parse/addr-space.C: New test.
        * g++.dg/parse/addr-space1.C: New test.
        * g++.dg/parse/addr-space2.C: New test.

# ------------------------ >8 ------------------------
diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index bb0544eeaea..ff1146ecc25 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -615,6 +615,33 @@ c_addr_space_name (addr_space_t as)
   return IDENTIFIER_POINTER (ridpointers [rid]);
 }
 
+/* Return true if between two named address spaces, whether there is a superset
+   named address space that encompasses both address spaces.  If there is a
+   superset, return which address space is the superset.  */
+
+bool
+addr_space_superset (addr_space_t as1, addr_space_t as2,
+		     addr_space_t * common)
+{
+  if (as1 == as2)
+    {
+      *common = as1;
+      return true;
+    }
+  else if (targetm.addr_space.subset_p (as1, as2))
+    {
+      *common = as2;
+      return true;
+    }
+  else if (targetm.addr_space.subset_p (as2, as1))
+    {
+      *common = as1;
+      return true;
+    }
+  else
+    return false;
+}
+
 /* Push current bindings for the function name VAR_DECLS.  */
 
 void
@@ -2809,6 +2836,25 @@ c_build_bitfield_integer_type (unsigned HOST_WIDE_INT width, int unsignedp)
   return build_nonstandard_integer_type (width, unsignedp);
 }
 
+/* Register reserved keyword WORD as qualifier for address space AS.  */
+
+void
+c_register_addr_space (const char *word, addr_space_t as)
+{
+  int rid = RID_FIRST_ADDR_SPACE + as;
+  tree id;
+
+  /* Address space qualifiers are only supported
+     in C with GNU extensions enabled.  */
+  if (c_dialect_objc () || flag_no_asm)
+    return;
+
+  id = get_identifier (word);
+  C_SET_RID_CODE (id, rid);
+  TREE_LANG_FLAG_0 (id) = 1;
+  ridpointers[rid] = id;
+}
+
 /* The C version of the register_builtin_type langhook.  */
 
 void
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 52a85bfb783..d36f9e4975b 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -829,12 +829,11 @@ extern const struct attribute_spec c_common_format_attribute_table[];
 
 extern tree (*make_fname_decl) (location_t, tree, int);
 
-/* In c-decl.cc and cp/tree.cc.  FIXME.  */
-extern void c_register_addr_space (const char *str, addr_space_t as);
-
 /* In c-common.cc.  */
 extern bool in_late_binary_op;
 extern const char *c_addr_space_name (addr_space_t as);
+extern const char *c_addr_space_name (addr_space_t as);
+extern bool addr_space_superset (addr_space_t, addr_space_t, addr_space_t *);
 extern tree identifier_global_value (tree);
 extern tree identifier_global_tag (tree);
 extern bool names_builtin_p (const char *);
@@ -952,6 +951,7 @@ extern void c_common_finish (void);
 extern void c_common_parse_file (void);
 extern FILE *get_dump_info (int, dump_flags_t *);
 extern alias_set_type c_common_get_alias_set (tree);
+extern void c_register_addr_space (const char *, addr_space_t);
 extern void c_register_builtin_type (tree, const char*);
 extern bool c_promoting_integer_type_p (const_tree);
 extern bool self_promoting_args_p (const_tree);
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index c701f07befe..e1bb4f1cf37 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -12337,25 +12337,6 @@ c_parse_final_cleanups (void)
   ext_block = NULL;
 }
 
-/* Register reserved keyword WORD as qualifier for address space AS.  */
-
-void
-c_register_addr_space (const char *word, addr_space_t as)
-{
-  int rid = RID_FIRST_ADDR_SPACE + as;
-  tree id;
-
-  /* Address space qualifiers are only supported
-     in C with GNU extensions enabled.  */
-  if (c_dialect_objc () || flag_no_asm)
-    return;
-
-  id = get_identifier (word);
-  C_SET_RID_CODE (id, rid);
-  C_IS_RESERVED_WORD (id) = 1;
-  ridpointers [rid] = id;
-}
-
 /* Return identifier to look up for omp declare reduction.  */
 
 tree
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index e130196a3a7..17185fd3da4 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -296,32 +296,6 @@ c_type_promotes_to (tree type)
   return type;
 }
 
-/* Return true if between two named address spaces, whether there is a superset
-   named address space that encompasses both address spaces.  If there is a
-   superset, return which address space is the superset.  */
-
-static bool
-addr_space_superset (addr_space_t as1, addr_space_t as2, addr_space_t *common)
-{
-  if (as1 == as2)
-    {
-      *common = as1;
-      return true;
-    }
-  else if (targetm.addr_space.subset_p (as1, as2))
-    {
-      *common = as2;
-      return true;
-    }
-  else if (targetm.addr_space.subset_p (as2, as1))
-    {
-      *common = as1;
-      return true;
-    }
-  else
-    return false;
-}
-
 /* Return a variant of TYPE which has all the type qualifiers of LIKE
    as well as those of TYPE.  */
 
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 72f4398a8f9..f1da04c6c58 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6206,6 +6206,7 @@ enum cp_decl_spec {
   ds_const,
   ds_volatile,
   ds_restrict,
+  ds_addr_space,
   ds_inline,
   ds_virtual,
   ds_explicit,
@@ -6278,6 +6279,8 @@ struct cp_decl_specifier_seq {
   /* True iff the alternate "__intN__" form of the __intN type has been
      used.  */
   BOOL_BITFIELD int_n_alt: 1;
+  /* The address space that the declaration belongs to.  */
+  addr_space_t address_space;
 };
 
 /* The various kinds of declarators.  */
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 9f78c500a15..16cc1917085 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -5280,6 +5280,8 @@ get_type_quals (const cp_decl_specifier_seq *declspecs)
     type_quals |= TYPE_QUAL_VOLATILE;
   if (decl_spec_seq_has_spec_p (declspecs, ds_restrict))
     type_quals |= TYPE_QUAL_RESTRICT;
+  if (decl_spec_seq_has_spec_p (declspecs, ds_addr_space))
+    type_quals |= ENCODE_QUAL_ADDR_SPACE (declspecs->address_space);
 
   return type_quals;
 }
@@ -5402,6 +5404,10 @@ check_tag_decl (cp_decl_specifier_seq *declspecs,
 	error_at (declspecs->locations[ds_restrict],
 		  "%<__restrict%> can only be specified for objects and "
 		  "functions");
+      else if (decl_spec_seq_has_spec_p (declspecs, ds_addr_space))
+	error_at (declspecs->locations[ds_addr_space],
+		  "address space can only be specified for objects and "
+		  "functions");
       else if (decl_spec_seq_has_spec_p (declspecs, ds_thread))
 	error_at (declspecs->locations[ds_thread],
 		  "%<__thread%> can only be specified for objects "
diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
index eb53e0ebeb4..06625ad9a4b 100644
--- a/gcc/cp/mangle.cc
+++ b/gcc/cp/mangle.cc
@@ -2509,6 +2509,15 @@ write_CV_qualifiers_for_type (const tree type)
      array.  */
   cp_cv_quals quals = TYPE_QUALS (type);
 
+  if (DECODE_QUAL_ADDR_SPACE (quals))
+    {
+      addr_space_t as = DECODE_QUAL_ADDR_SPACE (quals);
+      const char *as_name = c_addr_space_name (as);
+      write_char ('U');
+      write_unsigned_number (strlen (as_name));
+      write_string (as_name);
+      ++num_qualifiers;
+    }
   if (quals & TYPE_QUAL_RESTRICT)
     {
       write_char ('r');
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 763df6f479b..110ceafc98b 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -7640,6 +7640,14 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 		    postfix_expression = error_mark_node;
 		    break;
 		  }
+		if (type != error_mark_node
+		    && !ADDR_SPACE_GENERIC_P (TYPE_ADDR_SPACE (type))
+		    && current_function_decl)
+		  {
+		    error
+		      ("compound literal qualified by address-space qualifier");
+		    type = error_mark_node;
+		  }
 		/* Form the representation of the compound-literal.  */
 		postfix_expression
 		  = finish_compound_literal (type, initializer,
@@ -19408,6 +19416,15 @@ cp_parser_type_specifier (cp_parser* parser,
       break;
     }
 
+
+  if (RID_FIRST_ADDR_SPACE <= keyword && keyword <= RID_LAST_ADDR_SPACE)
+    {
+      ds = ds_addr_space;
+      if (is_cv_qualifier)
+	*is_cv_qualifier = true;
+    }
+
+
   /* Handle simple keywords.  */
   if (ds != ds_last)
     {
@@ -23776,6 +23793,7 @@ cp_parser_ptr_operator (cp_parser* parser,
    GNU Extension:
 
    cv-qualifier:
+     address-space-qualifier
      __restrict__
 
    Returns a bitmask representing the cv-qualifiers.  */
@@ -23812,6 +23830,13 @@ cp_parser_cv_qualifier_seq_opt (cp_parser* parser)
 	  break;
 	}
 
+      if (RID_FIRST_ADDR_SPACE <= token->keyword &&
+	  token->keyword <= RID_LAST_ADDR_SPACE)
+	{
+	  cv_qualifier =
+	    ENCODE_QUAL_ADDR_SPACE (token->keyword - RID_FIRST_ADDR_SPACE);
+	}
+
       if (!cv_qualifier)
 	break;
 
@@ -32705,6 +32730,8 @@ set_and_check_decl_spec_loc (cp_decl_specifier_seq *decl_specs,
       decl_specs->locations[ds] = location;
       if (ds == ds_thread)
 	decl_specs->gnu_thread_keyword_p = token_is__thread (token);
+      else if (ds == ds_addr_space)
+	decl_specs->address_space = token->keyword - RID_FIRST_ADDR_SPACE;
     }
   else
     {
@@ -32737,6 +32764,25 @@ set_and_check_decl_spec_loc (cp_decl_specifier_seq *decl_specs,
 	      error_at (&richloc, "duplicate %qD", token->u.value);
 	    }
 	}
+      else if (ds == ds_addr_space)
+	{
+	  addr_space_t as1 = decl_specs->address_space;
+	  addr_space_t as2 = token->keyword - RID_FIRST_ADDR_SPACE;
+
+	  gcc_rich_location richloc (location);
+	  richloc.add_fixit_remove ();
+	  if (!ADDR_SPACE_GENERIC_P (as1) && !ADDR_SPACE_GENERIC_P (as2)
+	      && as1 != as2)
+	    error_at (&richloc,
+		      "conflicting named address spaces (%s vs %s)",
+		      c_addr_space_name (as1), c_addr_space_name (as2));
+	  if (as1 == as2 && !ADDR_SPACE_GENERIC_P (as1))
+	    error_at (&richloc,
+		      "duplicate named address space %s",
+		      c_addr_space_name (as1));
+
+	  decl_specs->address_space = as2;
+	}
       else
 	{
 	  static const char *const decl_spec_names[] = {
diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 3b37567cbd7..5e14ac837fc 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -6048,15 +6048,6 @@ cp_free_lang_data (tree t)
     DECL_CHAIN (t) = NULL_TREE;
 }
 
-/* Stub for c-common.  Please keep in sync with c-decl.cc.
-   FIXME: If address space support is target specific, then this
-   should be a C target hook.  But currently this is not possible,
-   because this function is called via REGISTER_TARGET_PRAGMAS.  */
-void
-c_register_addr_space (const char * /*word*/, addr_space_t /*as*/)
-{
-}
-
 /* Return the number of operands in T that we care about for things like
    mangling.  */
 
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index ceb80d9744f..5d48920e687 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -656,10 +656,26 @@ composite_pointer_type (const op_location_t &location,
 	  else
 	    return error_mark_node;
         }
+      /* If possible merge the address space into the superset of the address
+	  spaces of t1 and t2, or raise an error. */
+      addr_space_t as_t1 = TYPE_ADDR_SPACE (t1);
+      addr_space_t as_t2 = TYPE_ADDR_SPACE (t2);
+      addr_space_t as_common;
+
+      /* If the two named address spaces are different, determine the common
+	 superset address space.  If there isn't one, raise an error.  */
+      if (!addr_space_superset (as_t1, as_t2, &as_common))
+	{
+	  as_common = as_t1;
+	  error_at (location,
+		    "%qT and %qT are in disjoint named address spaces",
+		    t1, t2);
+	}
       result_type
 	= cp_build_qualified_type (void_type_node,
-				   (cp_type_quals (TREE_TYPE (t1))
-				    | cp_type_quals (TREE_TYPE (t2))));
+				   (CLEAR_QUAL_ADDR_SPACE (cp_type_quals (TREE_TYPE (t1)))
+				    | CLEAR_QUAL_ADDR_SPACE (cp_type_quals (TREE_TYPE (t2)))
+				    | ENCODE_QUAL_ADDR_SPACE (as_common)));
       result_type = build_pointer_type (result_type);
       /* Merge the attributes.  */
       attributes = (*targetm.merge_type_attributes) (t1, t2);
@@ -10805,6 +10821,19 @@ comp_ptr_ttypes_real (tree to, tree from, int constp)
 	      to_more_cv_qualified = true;
 	    }
 
+      /* Warn about conversions between pointers to disjoint
+	 address spaces.  */
+      if (TREE_CODE (from) == POINTER_TYPE
+	  && TREE_CODE (to) == POINTER_TYPE)
+	{
+	  addr_space_t as_from = TYPE_ADDR_SPACE (TREE_TYPE (from));
+	  addr_space_t as_to = TYPE_ADDR_SPACE (TREE_TYPE (to));
+	  addr_space_t as_common;
+
+	  if (!addr_space_superset (as_to, as_from, &as_common))
+	    return false;
+	}
+
 	  if (constp > 0)
 	    constp &= TYPE_READONLY (to);
 	}
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 84e6f6694ab..c89df8778b2 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -1448,7 +1448,7 @@ Fixed-point types are supported by the DWARF debug information format.
 @section Named Address Spaces
 @cindex Named Address Spaces
 
-As an extension, GNU C supports named address spaces as
+As an extension, GNU C and GNU C++ support named address spaces as
 defined in the N1275 draft of ISO/IEC DTR 18037.  Support for named
 address spaces in GCC will evolve as the draft technical report
 changes.  Calling conventions for any target might also change.  At
diff --git a/gcc/testsuite/g++.dg/abi/mangle-addr-space1.C b/gcc/testsuite/g++.dg/abi/mangle-addr-space1.C
new file mode 100644
index 00000000000..c01f8d6054a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/mangle-addr-space1.C
@@ -0,0 +1,10 @@
+// { dg-do run { target { i?86-*-* x86_64-*-* } } }
+// { dg-options "-fabi-version=8 -Wabi -save-temps" }
+// { dg-final { scan-assembler "_Z1fPU8__seg_fsVi" } }
+
+int f (int volatile __seg_fs *a)
+{
+  return *a;
+}
+
+int main () {}
diff --git a/gcc/testsuite/g++.dg/parse/addr-space.C b/gcc/testsuite/g++.dg/parse/addr-space.C
new file mode 100644
index 00000000000..ebb6316054a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/addr-space.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+
+__seg_fs struct foo; // { dg-error "address space can only be specified for objects and functions" }
+
+int
+main ()
+{
+	return 0; 
+}
diff --git a/gcc/testsuite/g++.dg/parse/addr-space1.C b/gcc/testsuite/g++.dg/parse/addr-space1.C
new file mode 100644
index 00000000000..2e8ee32a885
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/addr-space1.C
@@ -0,0 +1,10 @@
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+// { dg-options "-std=gnu++98" }
+
+int
+main ()
+{
+	struct foo {int a; char b[2];} structure;
+	structure = ((__seg_fs struct foo) {1 + 2, 'a', 0}); // { dg-error "compound literal qualified by address-space qualifier" }
+	return 0; 
+}
diff --git a/gcc/testsuite/g++.dg/parse/addr-space2.C b/gcc/testsuite/g++.dg/parse/addr-space2.C
new file mode 100644
index 00000000000..5b2c0f28078
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/addr-space2.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+
+__seg_fs __seg_gs int *a; // { dg-error "conflicting named address spaces .__seg_fs vs __seg_gs." }
+
+int
+main ()
+{
+	return 0; 
+}
diff --git a/gcc/tree.h b/gcc/tree.h
index 8844471e9a5..b7da4c5141a 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -2229,7 +2229,7 @@ extern tree vector_element_bits_tree (const_tree);
 
 /* Encode/decode the named memory support as part of the qualifier.  If more
    than 8 qualifiers are added, these macros need to be adjusted.  */
-#define ENCODE_QUAL_ADDR_SPACE(NUM) ((NUM & 0xFF) << 8)
+#define ENCODE_QUAL_ADDR_SPACE(NUM) (((NUM) & 0xFF) << 8)
 #define DECODE_QUAL_ADDR_SPACE(X) (((X) >> 8) & 0xFF)
 
 /* Return all qualifiers except for the address space qualifiers.  */