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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: rjmccall.
aaron.ballman added inline comments.


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


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11778
+def err_wasm_reference_pr : Error<
+  "%select{pointers|references}0 to WebAssembly reference types are illegal">;
+def err_wasm_ca_reference : Error<
----------------
Rewording to avoid using "illegal" (and changes to the singular form).


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


================
Comment at: clang/lib/AST/ASTContext.cpp:4012-4018
+  if (Target->getTriple().isWasm() && Target->hasFeature("reference-types")) {
+#define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)                  \
+  if (BuiltinType::Id == BuiltinType::WasmExternRef)                           \
+    return SingletonId;
+#include "clang/Basic/WebAssemblyReferenceTypes.def"
+  }
+  return QualType();
----------------
Rather than returning a null type, should we assert we're in a wasm triple? We shouldn't be trying to form the type outside of a wasm target, right?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3147
+    type_name = MangledName;                                                   \
+    Out << (type_name == InternalName ? "u" : "") << type_name.size()          \
+        << type_name;                                                          \
----------------
aaron.ballman wrote:
> 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.
Still wondering about this, pinging @rjmccall for Itanium ABI expertise


================
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:
----------------
aaron.ballman wrote:
> Is it reasonable that this simply cannot mangle in Microsoft ABI mode?
Still wondering about this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122215