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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
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();
----------------
pmatos wrote:
> aaron.ballman wrote:
> > 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?
> asserting false with message rather than llvm_unreachable is the right way, correct?
I'd recommend something like:
```
assert(Target->getTriple().isWasm() && Target->hasFeature("reference-types") && "shouldn't try to generate type externref outsid of WebAssembly target");
#define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)                  \
  if (BuiltinType::Id == BuiltinType::WasmExternRef)                           \
    return SingletonId;
#include "clang/Basic/WebAssemblyReferenceTypes.def"
  }
llvm_unreachable("couldn't find the externref type?");
```
so it's clear that you shouldn't call this outside of a wasm target and that it's not an option to not find the 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:
----------------
pmatos wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Is it reasonable that this simply cannot mangle in Microsoft ABI mode?
> > Still wondering about this
> @aaron.ballman Why wouldn't externref_t be able to mangle in Microsoft ABI mode? Quite clueless when it comes to ABIs.
Including the wasm types here means that in Microsoft mode, use of those types in a context which requires mangling will diagnose (see this snippet just below):
```
    unsigned DiagID = Diags.getCustomDiagID(
        DiagnosticsEngine::Error, "cannot mangle this built-in %0 type yet");
    Diags.Report(Range.getBegin(), DiagID)
        << T->getName(Context.getASTContext().getPrintingPolicy()) << Range;
```
So I'm wondering whether you consider that to be reasonable behavior or not. If you don't want the diagnostic, then you should add a vendor extension mangling for the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122215