[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 17 07:15:29 PDT 2022
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Same question here about an RFC for adding this new type to the type system and a design document for how the type behaves as in https://reviews.llvm.org/D122215.
================
Comment at: clang/include/clang/Basic/Attr.td:4053
+ let Spellings = [Keyword<"__funcref">];
+ let Documentation = [Undocumented];
+}
----------------
tlively wrote:
> It would be good to document this!
It's a requirement that this be documented. Also, this should probably have a `Subjects` line that expects a function of some sort? (It doesn't impact the semantics of anything, but it self-documents the expectations for people reading Attr.td.)
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7332
+def err_attribute_webassembly_funcref : Error<
+ "'__clang_webassembly_funcref' attribute can only be applied to function pointer types">;
----------------
1) What is `__clang_webassembly_funcref`? Did you mean `__funcref`?
2) There's no test coverage for this diagnostic.
================
Comment at: clang/include/clang/Basic/TokenKinds.def:666
+// WebAssembly Type Extension
+KEYWORD(__funcref , KEYALL)
+
----------------
Why is this a keyword in all language modes?
================
Comment at: clang/include/clang/Parse/Parser.h:2843
void ParseMicrosoftTypeAttributes(ParsedAttributes &attrs);
+ void ParseWebAssemblyFuncrefTypeAttribute(ParsedAttributes &attrs);
void DiagnoseAndSkipExtendedMicrosoftTypeAttributes();
----------------
================
Comment at: clang/include/clang/Sema/Sema.h:13103-13104
CallExpr *TheCall);
+ bool CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
+ CallExpr *TheCall);
----------------
Formatting is off here.
================
Comment at: clang/include/clang/Sema/Sema.h:13169
+ // WebAssembly builtin handling
+ bool SemaBuiltinWasmRefNullFunc(CallExpr *TheCall);
+
----------------
I think there's a verb missing from this function identifier -- also, I don't think we need to continue the terrible trend of prepending `Sema` onto the function name which lives in the `Sema` object.
================
Comment at: clang/lib/AST/ASTContext.cpp:954
10, // wasm_externref,
+ 20, // wasm_funcref
};
----------------
Where did this value come from?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:845-846
+void Parser::ParseWebAssemblyFuncrefTypeAttribute(ParsedAttributes &attrs) {
+ if (Tok.getKind() == tok::kw___funcref) {
+ IdentifierInfo *AttrName = Tok.getIdentifierInfo();
----------------
================
Comment at: clang/lib/Parse/ParseDecl.cpp:851-854
+ } else {
+ return;
+ }
+}
----------------
================
Comment at: clang/lib/Parse/ParseDecl.cpp:5780
+ case tok::kw___funcref:
+ if (AttrReqs & AR_DeclspecAttributesParsed) {
+ ParseWebAssemblyFuncrefTypeAttribute(DS.getAttributes());
----------------
Why that specific ordering as opposed to `AR_VendorAttributesParsed`?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:6557-6561
+ // Therefore we need to change the types of the DeclRefExpr (stored in FDecl)
+ // and regenerate a straight up CallExpr on the modified FDecl.
+ // returning
+ // CallExpr
+ // `- FunctionDecl
----------------
Why would we need to do this?? And what do we do when there is no `FunctionDecl` to get back to (because it's a call expression through a function pointer, for example)?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:6564
+ // Prepare FDecl type
+ QualType Pointee = Context.getFunctionType(Context.VoidTy, {}, {});
+ QualType Type = Context.getPointerType(Pointee);
----------------
Why are you guaranteed this function returns void?
================
Comment at: clang/lib/Sema/SemaType.cpp:7256
+ QualType Desugared = Type;
+ const AttributedType *AT = dyn_cast<AttributedType>(Type);
+ while (AT) {
----------------
================
Comment at: clang/lib/Sema/SemaType.cpp:7269
+ }
+ Attrs[NewAttrKind] = true;
+
----------------
This seems like it's not used.
================
Comment at: clang/lib/Sema/SemaType.cpp:7289-7290
+ QualType Pointee = Type->getPointeeType();
+ Pointee = S.Context.getAddrSpaceQualType(
+ S.Context.removeAddrSpaceQualType(Pointee), ASIdx);
+ Type = State.getAttributedType(A, Type, S.Context.getPointerType(Pointee));
----------------
What happens when the user's function already has an explicitly specified address space?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128440/new/
https://reviews.llvm.org/D128440
More information about the cfe-commits
mailing list