[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang

[Aaron Ballman via Phabricator via llvm-commits]

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