-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[WebAssembly,clang] Add __builtin_wasm_test_function_pointer_signature #150201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e clang intrinsic Tests if the runtime type of the function pointer matches the static type. If this returns false, calling the function pointer will trap. Uses `@llvm.wasm.ref.test.func` added in llvm#147486.
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Hood Chatham (hoodmane) ChangesTests if the runtime type of the function pointer matches the static type. If this returns false, calling the function pointer will trap. Uses cc @sbc100 @dschuff @tlively Full diff: https://github.com/llvm/llvm-project/pull/150201.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/BuiltinsWebAssembly.def b/clang/include/clang/Basic/BuiltinsWebAssembly.def
index e2afcc08064b2..1e03a40b1a22b 100644
--- a/clang/include/clang/Basic/BuiltinsWebAssembly.def
+++ b/clang/include/clang/Basic/BuiltinsWebAssembly.def
@@ -199,6 +199,12 @@ TARGET_BUILTIN(__builtin_wasm_ref_is_null_extern, "ii", "nct", "reference-types"
// return type.
TARGET_BUILTIN(__builtin_wasm_ref_null_func, "i", "nct", "reference-types")
+// Check if the static type of a function pointer matches its static type. Used
+// to avoid "function signature mismatch" traps. Takes a function pointer, uses
+// table.get to look up the pointer in __indirect_function_table and then
+// ref.test to test the type.
+TARGET_BUILTIN(__builtin_wasm_test_function_pointer_signature, "i.", "nct", "reference-types")
+
// Table builtins
TARGET_BUILTIN(__builtin_wasm_table_set, "viii", "t", "reference-types")
TARGET_BUILTIN(__builtin_wasm_table_get, "iii", "t", "reference-types")
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b2ea65ae111be..745ac8cb5dc2e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7575,6 +7575,8 @@ def err_typecheck_illegal_increment_decrement : Error<
"cannot %select{decrement|increment}1 value of type %0">;
def err_typecheck_expect_int : Error<
"used type %0 where integer is required">;
+def err_typecheck_expect_function_pointer
+ : Error<"used type %0 where function pointer is required">;
def err_typecheck_expect_hlsl_resource : Error<
"used type %0 where __hlsl_resource_t is required">;
def err_typecheck_arithmetic_incomplete_or_sizeless_type : Error<
@@ -13202,6 +13204,10 @@ def err_wasm_builtin_arg_must_match_table_element_type : Error <
"%ordinal0 argument must match the element type of the WebAssembly table in the %ordinal1 argument">;
def err_wasm_builtin_arg_must_be_integer_type : Error <
"%ordinal0 argument must be an integer">;
+def err_wasm_builtin_test_fp_sig_cannot_include_reference_type
+ : Error<"not supported for "
+ "function pointers with a reference type %select{return "
+ "value|parameter}0">;
// OpenACC diagnostics.
def warn_acc_routine_unimplemented
diff --git a/clang/include/clang/Sema/SemaWasm.h b/clang/include/clang/Sema/SemaWasm.h
index 2123e073516cb..8c0639fd7e76f 100644
--- a/clang/include/clang/Sema/SemaWasm.h
+++ b/clang/include/clang/Sema/SemaWasm.h
@@ -37,6 +37,7 @@ class SemaWasm : public SemaBase {
bool BuiltinWasmTableGrow(CallExpr *TheCall);
bool BuiltinWasmTableFill(CallExpr *TheCall);
bool BuiltinWasmTableCopy(CallExpr *TheCall);
+ bool BuiltinWasmTestFunctionPointerSignature(CallExpr *TheCall);
WebAssemblyImportNameAttr *
mergeImportNameAttr(Decl *D, const WebAssemblyImportNameAttr &AL);
diff --git a/clang/lib/CodeGen/TargetBuiltins/WebAssembly.cpp b/clang/lib/CodeGen/TargetBuiltins/WebAssembly.cpp
index b7fd70e855d40..5644792028013 100644
--- a/clang/lib/CodeGen/TargetBuiltins/WebAssembly.cpp
+++ b/clang/lib/CodeGen/TargetBuiltins/WebAssembly.cpp
@@ -12,7 +12,10 @@
#include "CGBuiltin.h"
#include "clang/Basic/TargetBuiltins.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/IR/Constants.h"
#include "llvm/IR/IntrinsicsWebAssembly.h"
+#include "llvm/Support/ErrorHandling.h"
using namespace clang;
using namespace CodeGen;
@@ -218,6 +221,60 @@ Value *CodeGenFunction::EmitWebAssemblyBuiltinExpr(unsigned BuiltinID,
Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_ref_null_func);
return Builder.CreateCall(Callee);
}
+ case WebAssembly::BI__builtin_wasm_test_function_pointer_signature: {
+ Value *FuncRef = EmitScalarExpr(E->getArg(0));
+
+ // Get the function type from the argument's static type
+ QualType ArgType = E->getArg(0)->getType();
+ const PointerType *PtrTy = ArgType->getAs<PointerType>();
+ assert(PtrTy && "Sema should have ensured this is a function pointer");
+
+ const FunctionType *FuncTy = PtrTy->getPointeeType()->getAs<FunctionType>();
+ assert(FuncTy && "Sema should have ensured this is a function pointer");
+
+ // In the llvm IR, we won't have access anymore to the type of the function
+ // pointer so we need to insert this type information somehow. We gave the
+ // @llvm.wasm.ref.test.func varargs and here we add an extra 0 argument of
+ // the type corresponding to the type of each argument of the function
+ // signature. When we lower from the IR we'll use the types of these
+ // arguments to determine the signature we want to test for.
+
+ // Make a type index constant with 0. This gets replaced by the actual type
+ // in WebAssemblyMCInstLower.cpp.
+ llvm::FunctionType *LLVMFuncTy =
+ cast<llvm::FunctionType>(ConvertType(QualType(FuncTy, 0)));
+
+ uint NParams = LLVMFuncTy->getNumParams();
+ std::vector<Value *> Args;
+ Args.reserve(NParams + 2);
+ // The only real argument is the FuncRef
+ Args.push_back(FuncRef);
+
+ // Add the type information
+ auto addType = [&Args](llvm::Type *T) {
+ if (T->isVoidTy()) {
+ // Do nothing
+ } else if (T->isFloatingPointTy()) {
+ Args.push_back(ConstantFP::get(T, 0));
+ } else if (T->isIntegerTy()) {
+ Args.push_back(ConstantInt::get(T, 0));
+ } else {
+ // TODO: Handle reference types here. For now, we reject them in Sema.
+ llvm_unreachable("Unhandled type");
+ }
+ };
+
+ addType(LLVMFuncTy->getReturnType());
+ // The token type indicates the boundary between return types and param
+ // types.
+ Args.push_back(
+ PoisonValue::get(llvm::Type::getTokenTy(getLLVMContext())));
+ for (uint i = 0; i < NParams; i++) {
+ addType(LLVMFuncTy->getParamType(i));
+ }
+ Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_ref_test_func);
+ return Builder.CreateCall(Callee, Args);
+ }
case WebAssembly::BI__builtin_wasm_swizzle_i8x16: {
Value *Src = EmitScalarExpr(E->getArg(0));
Value *Indices = EmitScalarExpr(E->getArg(1));
diff --git a/clang/lib/Sema/SemaWasm.cpp b/clang/lib/Sema/SemaWasm.cpp
index 6faea24a46b09..8998492a71619 100644
--- a/clang/lib/Sema/SemaWasm.cpp
+++ b/clang/lib/Sema/SemaWasm.cpp
@@ -227,6 +227,53 @@ bool SemaWasm::BuiltinWasmTableCopy(CallExpr *TheCall) {
return false;
}
+bool SemaWasm::BuiltinWasmTestFunctionPointerSignature(CallExpr *TheCall) {
+ if (SemaRef.checkArgCount(TheCall, 1))
+ return true;
+
+ Expr *FuncPtrArg = TheCall->getArg(0);
+ QualType ArgType = FuncPtrArg->getType();
+
+ // Check that the argument is a function pointer
+ const PointerType *PtrTy = ArgType->getAs<PointerType>();
+ if (!PtrTy) {
+ return Diag(FuncPtrArg->getBeginLoc(),
+ diag::err_typecheck_expect_function_pointer)
+ << ArgType << FuncPtrArg->getSourceRange();
+ }
+
+ const FunctionProtoType *FuncTy =
+ PtrTy->getPointeeType()->getAs<FunctionProtoType>();
+ if (!FuncTy) {
+ return Diag(FuncPtrArg->getBeginLoc(),
+ diag::err_typecheck_expect_function_pointer)
+ << ArgType << FuncPtrArg->getSourceRange();
+ }
+
+ // Check that the function pointer doesn't use reference types
+ if (FuncTy->getReturnType().isWebAssemblyReferenceType()) {
+ return Diag(
+ FuncPtrArg->getBeginLoc(),
+ diag::err_wasm_builtin_test_fp_sig_cannot_include_reference_type)
+ << 0 << FuncTy->getReturnType() << FuncPtrArg->getSourceRange();
+ }
+ auto NParams = FuncTy->getNumParams();
+ for (unsigned I = 0; I < NParams; I++) {
+ if (FuncTy->getParamType(I).isWebAssemblyReferenceType()) {
+ return Diag(
+ FuncPtrArg->getBeginLoc(),
+ diag::
+ err_wasm_builtin_test_fp_sig_cannot_include_reference_type)
+ << 1 << FuncPtrArg->getSourceRange();
+ }
+ }
+
+ // Set return type to int (the result of the test)
+ TheCall->setType(getASTContext().IntTy);
+
+ return false;
+}
+
bool SemaWasm::CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI,
unsigned BuiltinID,
CallExpr *TheCall) {
@@ -249,6 +296,8 @@ bool SemaWasm::CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI,
return BuiltinWasmTableFill(TheCall);
case WebAssembly::BI__builtin_wasm_table_copy:
return BuiltinWasmTableCopy(TheCall);
+ case WebAssembly::BI__builtin_wasm_test_function_pointer_signature:
+ return BuiltinWasmTestFunctionPointerSignature(TheCall);
}
return false;
diff --git a/clang/test/CodeGen/builtins-wasm.c b/clang/test/CodeGen/builtins-wasm.c
index d8aff82b0c140..d52f5f88ed8cb 100644
--- a/clang/test/CodeGen/builtins-wasm.c
+++ b/clang/test/CodeGen/builtins-wasm.c
@@ -751,3 +751,27 @@ void *tp (void) {
return __builtin_thread_pointer ();
// WEBASSEMBLY: call {{.*}} @llvm.thread.pointer.p0()
}
+
+
+typedef void (*funcref_t)();
+typedef int (*funcref_int_t)(int);
+typedef float (*F1)(float, double, int);
+typedef int (*F2)(float, double, int);
+typedef int (*F3)(int, int, int);
+typedef void (*F4)(int, int, int);
+typedef void (*F5)(void);
+
+void use(int);
+
+void test_function_pointer_signature_void(F1 func) {
+ // WEBASSEMBLY: %0 = tail call i32 (ptr, ...) @llvm.wasm.ref.test.func(ptr %func, float 0.000000e+00, token poison, float 0.000000e+00, double 0.000000e+00, i32 0)
+ use(__builtin_wasm_test_function_pointer_signature(func));
+ // WEBASSEMBLY: %1 = tail call i32 (ptr, ...) @llvm.wasm.ref.test.func(ptr %func, i32 0, token poison, float 0.000000e+00, double 0.000000e+00, i32 0)
+ use(__builtin_wasm_test_function_pointer_signature((F2)func));
+ // WEBASSEMBLY: %2 = tail call i32 (ptr, ...) @llvm.wasm.ref.test.func(ptr %func, i32 0, token poison, i32 0, i32 0, i32 0)
+ use(__builtin_wasm_test_function_pointer_signature((F3)func));
+ // WEBASSEMBLY: %3 = tail call i32 (ptr, ...) @llvm.wasm.ref.test.func(ptr %func, token poison, i32 0, i32 0, i32 0)
+ use(__builtin_wasm_test_function_pointer_signature((F4)func));
+ // WEBASSEMBLY: %4 = tail call i32 (ptr, ...) @llvm.wasm.ref.test.func(ptr %func, token poison)
+ use(__builtin_wasm_test_function_pointer_signature((F5)func));
+}
diff --git a/clang/test/Sema/builtins-wasm.c b/clang/test/Sema/builtins-wasm.c
index 31e5291d3ae5e..a3486b1aedb13 100644
--- a/clang/test/Sema/builtins-wasm.c
+++ b/clang/test/Sema/builtins-wasm.c
@@ -54,3 +54,27 @@ void test_table_copy(int dst_idx, int src_idx, int nelem) {
__builtin_wasm_table_copy(table, table, dst_idx, src_idx, table); // expected-error {{5th argument must be an integer}}
__builtin_wasm_table_copy(table, table, dst_idx, src_idx, nelem);
}
+
+typedef void (*F1)(void);
+typedef int (*F2)(int);
+typedef int (*F3)(__externref_t);
+typedef __externref_t (*F4)(int);
+
+void test_function_pointer_signature() {
+ // Test argument count validation
+ (void)__builtin_wasm_test_function_pointer_signature(); // expected-error {{too few arguments to function call, expected 1, have 0}}
+ (void)__builtin_wasm_test_function_pointer_signature((F1)0, (F2)0); // expected-error {{too many arguments to function call, expected 1, have 2}}
+
+ // // Test argument type validation - should require function pointer
+ (void)__builtin_wasm_test_function_pointer_signature((void*)0); // expected-error {{used type 'void *' where function pointer is required}}
+ (void)__builtin_wasm_test_function_pointer_signature((int)0); // expected-error {{used type 'int' where function pointer is required}}
+ (void)__builtin_wasm_test_function_pointer_signature((F3)0); // expected-error {{not supported for function pointers with a reference type parameter}}
+ (void)__builtin_wasm_test_function_pointer_signature((F4)0); // expected-error {{not supported for function pointers with a reference type return value}}
+
+ // // Test valid usage
+ int res = __builtin_wasm_test_function_pointer_signature((F1)0);
+ res = __builtin_wasm_test_function_pointer_signature((F2)0);
+
+ // Test return type
+ _Static_assert(EXPR_HAS_TYPE(__builtin_wasm_test_function_pointer_signature((F1)0), int), "");
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think we might be able to use this in a few places in emscripten, to avoid jumping through JS.
Kind of interesting that this only takes a single argument.. intuitively I would expect two, but I guess this makes sense.
Would the name __builtin_wasm_test_function_pointer_type also make sense? I'm not sure about type or signature is commonly used here.
…#150116) PR #147486 broke the sanitizer and expensive-checks buildbot. These captures were needed when toWasmValType emitted a diagnostic but are no longer needed since we changed it to an assertion failure. This removes the unneeded captures and should fix the sanitizer-buildbot. I also fixed the codegen in the wasm64 target: table.get requires an i32 but in wasm64 the function pointer is an i64. We need an additional `i32.wrap_i64` to convert it. I also added `-verify-machineinstrs` to the tests so that the test suite validates this fix. Finally, I noticed that #150201 uses a feature of the intrinsic that is not covered by the tests, namely `ptr` arguments. So I added one additional test case to ensure that it works properly. cc @dschuff
|
I'm neutral on the name, happy to change it if you prefer. |
|
|
|
I'd also like to add |
Where would the return value go here? Would end up in &success? What happens if none of the signatures match? Could you write this in terms of |
|
My thought is that it returns the return value. If one of the signatures matches it sets |
How would it work for functions that return different types? i.e. how would |
|
It seems that builtins can do that. In |
Thats seems really magical.. |
|
Well it would also be possible to make success the return value, and make the first argument a |
|
Other builtins with dynamic return type:
|
|
This change looks fine. |
|
If this change looks good can we merge it? |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/24106 Here is the relevant piece of the build log for the reference |
|
Thanks again @dschuff ! |
…llvm#150116) PR llvm#147486 broke the sanitizer and expensive-checks buildbot. These captures were needed when toWasmValType emitted a diagnostic but are no longer needed since we changed it to an assertion failure. This removes the unneeded captures and should fix the sanitizer-buildbot. I also fixed the codegen in the wasm64 target: table.get requires an i32 but in wasm64 the function pointer is an i64. We need an additional `i32.wrap_i64` to convert it. I also added `-verify-machineinstrs` to the tests so that the test suite validates this fix. Finally, I noticed that llvm#150201 uses a feature of the intrinsic that is not covered by the tests, namely `ptr` arguments. So I added one additional test case to ensure that it works properly. cc @dschuff
llvm#150201) Tests if the runtime type of the function pointer matches the static type. If this returns false, calling the function pointer will trap. Uses `@llvm.wasm.ref.test.func` added in llvm#147486. Also adds a "gc" wasm feature to gate the use of the ref.test instruction.
| if (Feature == "-gc") { | ||
| HasGC = false; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we add new feature like this don't we normally add them to WebAssemblyTargetInfo::initFeatureMap in addBleedingEdgeFeatures.. I would also expect the test for bleeding edge CPU to be updated, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test for llvm/test/CodeGen/WebAssembly/target-features-cpus.ll below does seem to include gc in bleeding-edge. But you're right, that we've been also adding things with addBleedingEdgeFeatures so it's weird that that test changes. We should look into what's going on there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #151107
…ures (#151107) See suggestion here: llvm/llvm-project#150201 (comment)
Also alphebetize feature list, add `-mgc` and `-mno-gc` flags, and add some missing feature tests. Reland of llvm#151107. llvm#150201 (comment)
Also alphebetize feature list, add `-mgc` and `-mno-gc` flags, and add some missing feature tests. Reland of #151107. #150201 (comment)
…ures (#151294) Also alphebetize feature list, add `-mgc` and `-mno-gc` flags, and add some missing feature tests. Reland of #151107. llvm/llvm-project#150201 (comment)
…or Emscripten trampolines Since llvm/llvm-project#150201 was merged, there is now a better way to do this. Requires Emscripten 4.0.12.
…or Emscripten trampolines Since llvm/llvm-project#150201 was merged, there is now a better way to do this. Requires Emscripten 4.0.12.
…cripten trampoline (#137470) With llvm/llvm-project#150201 being merged, there is now a better way to generate the Emscripten trampoline, instead of including hand-generated binary WASM content. Requires Emscripten 4.0.12.
…or Emscripten trampoline (pythonGH-137470) With llvm/llvm-project#150201 being merged, there is now a better way to generate the Emscripten trampoline, instead of including hand-generated binary WASM content. Requires Emscripten 4.0.12. (cherry picked from commit 2629ee4) Co-authored-by: Hood Chatham <[email protected]>
…for Emscripten trampoline (GH-137470) (#139039) gh-128627: Use __builtin_wasm_test_function_pointer_signature for Emscripten trampoline (GH-137470) With llvm/llvm-project#150201 being merged, there is now a better way to generate the Emscripten trampoline, instead of including hand-generated binary WASM content. Requires Emscripten 4.0.12. (cherry picked from commit 2629ee4) Co-authored-by: Hood Chatham <[email protected]>
Tests if the runtime type of the function pointer matches the static type. If this returns false, calling the function pointer will trap. Uses
@llvm.wasm.ref.test.funcadded in #147486.Also adds a "gc" wasm feature to gate the use of the ref.test instruction.