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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
