This patch introduces a new type externref_t that denotes a WebAssembly opaque
reference type. It also implements builtin builtin_wasm_ref_null_extern(),
that returns a null value of __externref_t. This lays the ground work
for further builtins and reference types.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I have posted a new patch that fixed this. Can you please try it again and see if it works as you expect?
Thanks for the quick update, it works now!
Does this affect the debugging experience of the generated wasm programs? If you force mem2reg to run for all programs, no function arguments will be written to the "stack" referenced by the __stack_pointer global variable anymore. I don't know whether any wasm runtime actually uses that for debugging, though.
clang/include/clang/AST/ASTContext.h | ||
---|---|---|
1481 | getWebAssemblyExternrefType ? | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
11802 | Rewording to avoid using "illegal" (and changes to the singular form). | |
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
17 | Still wondering about this. | |
clang/lib/AST/ASTContext.cpp | ||
4090–4096 | 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? | |
clang/lib/AST/ItaniumMangle.cpp | ||
3237 | Still wondering about this, pinging @rjmccall for Itanium ABI expertise | |
clang/lib/AST/MicrosoftMangle.cpp | ||
2499–2500 | Still wondering about this |
Deal with Itanium Mangle and assert false outside Wasm triple.
clang/lib/AST/MicrosoftMangle.cpp | ||
---|---|---|
2499–2500 | @aaron.ballman Why wouldn't externref_t be able to mangle in Microsoft ABI mode? Quite clueless when it comes to ABIs. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
4090–4096 | 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. | |
clang/lib/AST/MicrosoftMangle.cpp | ||
2499–2500 | 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. |
clang/lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
825–826 | This is the creation of the debug info for an opaque structure type, although I guess we could use other representations for externref, do you have any suggestions? | |
clang/test/CodeGen/WebAssembly/wasm-externref.c | ||
12–14 | Not at the moment. There's not much you can do with externref without using tables for example which is in another patch. Happy to remove this test as we have something similar indeed in builtins-wasm.c. |
@aaron.ballman many thanks for the thorough reviews on the patches. Happy to see this moving in the right direction.
clang/lib/AST/MicrosoftMangle.cpp | ||
---|---|---|
2499–2500 | Oh - now I understand your point. I had forgotten I had initially written this 'workaround' to worry with later. Yes, let me add a vendor extension mangling for Microsoft ABI. |
Now under the correct revision: @aaron.ballman Added support for externref mangling in Microsoft. This follows the pattern implemented by OpenCL types.
I feel like the latest patch addresses all concerns until now. What do you think?
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
---|---|---|
17 | This is just a generic name to base the actual mangling on tbh. |
Aside from a nit about comments, I don't have any further concerns. LGTM!
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
---|---|---|
17 | Can you clarify the comment then? I'd expect MangledName to be an actual mangling, otherwise, and that's why I was wondering "which mangling?". |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
4090–4096 | In a non-assert build, doing just assert(false) causes the build to fail if you use -Werror -Wreturn-type. clang/lib/AST/ASTContext.cpp:4097:1: error: non-void function does not return a value in all control paths [-Werror,-Wreturn-type] |
@pmatos and I have tried and failed to reproduce the assert in the stage3 msan build locally (both of us on separate machines, different environments). We've reached out to @vitalybuka via email for any help in reproducing, because we're somewhat stumped at the moment.