[PATCH] D122215: [WebAssembly] Initial support for reference type externref in clang

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Adding new types to the type system is quite invasive; was there an RFC for this you can point me to along with a design document? I have no idea how to review this because I don't know what the semantics of this type are supposed to be.



================
Comment at: clang/include/clang/AST/ASTContext.h:1496
 
+  /// Return a WebAssembly externref type
+  QualType getExternrefType() const;
----------------



================
Comment at: clang/include/clang/AST/Type.h:1972-1973
+  /// Check if this is a WebAssembly Reference Type.
+  bool isWebAssemblyReferenceType() const;
+  bool isWebAssemblyExternrefType() const;
   /// Determines if this is a sizeless type supported by the
----------------
It's unfortunate to name this with `ReferenceType` given that we already have a considerable number of APIs that assume "reference type" to mean `&` or `&&`. We run into similar problems with "pointer type" and objective-c pointers.

Basically, I worry we're setting ourselves up for another `isObjCObjectPointerType()`/`isPointerType()` situation.


================
Comment at: clang/include/clang/Basic/AddressSpaces.h:59-60
 
+  wasm_var,
+  wasm_externref,
   // This denotes the count of language-specific address spaces and also
----------------
tlively wrote:
> What is `wasm_var`? It would be good to have a short comment here (and newline afterward).
Agreed; I'm not certain why `wasm_var` is part of this changeset.


================
Comment at: clang/include/clang/Basic/Attr.td:3937-3944
+def WebAssemblyFuncref : TypeAttr, TargetSpecificAttr<TargetWebAssembly> {
+  let Spellings = [Clang<"funcref">];
+  let Args = [];
+  let Documentation = [Undocumented];
+  // Represented as part of the enclosing function pointer type.
+  let ASTNode = 0;
+}
----------------
No new undocumented attributes, please. No need for the `Args` field, and please add the newline back to the end of the file.


================
Comment at: clang/include/clang/Basic/Builtins.def:50
 //  p -> pid_t
+//  e -> wasm externref
 //  . -> "...".  This may only occur at the end of the function list.
----------------
tlively wrote:
> 
How often do you expect to use this marking? If it's only going to be used once or twice, it might make sense to not steal a letter but instead use custom checking for the signature.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11683-11686
+def err_wasm_reference_pointer : Error<
+  "pointers to WebAssembly reference types are illegal">;
+def err_wasm_reference_reference : Error<
+  "references to WebAssembly reference types are illegal">;
----------------
These can be combined with a `%select`


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11687-11690
+def err_wasm_capture_reference : Error<
+  "cannot capture WebAssembly reference">;
+def err_wasm_addrof_reference : Error<
+  "cannot take address of WebAssembly reference">;
----------------
These can also be combined with a `%select`


================
Comment at: clang/include/clang/Basic/WebAssemblyReferenceTypes.def:16
+//
+//  - Name is the name of the builtin type.  MangledName is the mangled name.
+//
----------------
What kind of mangling is this name? Itanium? Microsoft? Something else?


================
Comment at: clang/lib/AST/ASTContext.cpp:952-953
+        12, // ptr64
+        1,  // wasm_var
+        10, // wasm_externref,
     };
----------------
How did you arrive at these values?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3147
+    type_name = MangledName;                                                   \
+    Out << (type_name == InternalName ? "u" : "") << type_name.size()          \
+        << type_name;                                                          \
----------------
tlively wrote:
> Our `MangledName` is not the same as our `InternalName`, so it looks like this condition will never be true. Should be follow the simpler pattern from the previous two targets instead?
I'm not an Itanium mangling expert, but doesn't this *have* to use the `u` marking per https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.builtin-type because this is definitely a vendor extension type.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:2480-2481
 #include "clang/Basic/RISCVVTypes.def"
+#define WASM_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
+#include "clang/Basic/WebAssemblyReferenceTypes.def"
   case BuiltinType::ShortAccum:
----------------
Is it reasonable that this simply cannot mangle in Microsoft ABI mode?


================
Comment at: clang/lib/AST/Type.cpp:2330-2333
+  const BuiltinType *BT = getAs<BuiltinType>();
+  if (BT && BT->getKind() == BuiltinType::WasmExternRef)
+    return true;
+  return false;
----------------



================
Comment at: clang/lib/Sema/SemaType.cpp:2179-2184
+  if (getASTContext().getTargetInfo().getTriple().isWasm()) {
+    if (T->isWebAssemblyReferenceType()) {
+      Diag(Loc, diag::err_wasm_reference_pointer);
+      return QualType();
+    }
+  }
----------------



================
Comment at: clang/lib/Sema/SemaType.cpp:2262-2267
+  if (getASTContext().getTargetInfo().getTriple().isWasm()) {
+    if (T->isWebAssemblyReferenceType()) {
+      Diag(Loc, diag::err_wasm_reference_reference);
+      return QualType();
+    }
+  }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122215/new/

https://reviews.llvm.org/D122215