From 015c899297c9fdc8ef9d0d22af29a7a0d0bdbc5c Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Tue, 26 May 2020 17:16:25 +0200 Subject: [PATCH 1/2] Make align expr on fns a compile error in Wasm In Wasm, specifying alignment of function pointers makes little sense since function pointers are in fact indices to a Wasm table, therefore any alignment check on those is invalid. This can cause unexpected behaviour when checking expected alignment with `@ptrToInt(fn_ptr)` or similar. This commit proposes to make `align` expressions a compile error when compiled to Wasm architecture. Some references: [1] [Mozilla: WebAssembly Tables](https://developer.mozilla.org/en-US/docs/WebAssembly/Understanding_the_text_format#WebAssembly_tables) [2] [Sunfishcode's Wasm Ref Manual](https://github.com/sunfishcode/wasm-reference-manual/blob/master/WebAssembly.md#indirect-call) --- src/analyze.cpp | 15 +++++++++++++++ test/stage1/behavior/align.zig | 17 +++++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index 88f967240a..ff1946c70e 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -1921,6 +1921,21 @@ static ZigType *analyze_fn_type(CodeGen *g, AstNode *proto_node, Scope *child_sc } if (fn_proto->align_expr != nullptr) { + if (target_is_wasm(g->zig_target)) { + // In Wasm, specifying alignment of function pointers makes little sense + // since function pointers are in fact indices to a Wasm table, therefore + // any alignment check on those is invalid. This can cause unexpected + // behaviour when checking expected alignment with `@ptrToInt(fn_ptr)` + // or similar. This commit proposes to make `align` expressions a + // compile error when compiled to Wasm architecture. + // + // Some references: + // [1] [Mozilla: WebAssembly Tables](https://developer.mozilla.org/en-US/docs/WebAssembly/Understanding_the_text_format#WebAssembly_tables) + // [2] [Sunfishcode's Wasm Ref Manual](https://github.com/sunfishcode/wasm-reference-manual/blob/master/WebAssembly.md#indirect-call) + add_node_error(g, fn_proto->align_expr, + buf_sprintf("align(N) expr is not allowed on function prototypes in wasm32/wasm64")); + return g->builtin_types.entry_invalid; + } if (!analyze_const_align(g, child_scope, fn_proto->align_expr, &fn_type_id.alignment)) { return g->builtin_types.entry_invalid; } diff --git a/test/stage1/behavior/align.zig b/test/stage1/behavior/align.zig index 2d0e8f0982..62f439d6df 100644 --- a/test/stage1/behavior/align.zig +++ b/test/stage1/behavior/align.zig @@ -25,6 +25,9 @@ fn noop1() align(1) void {} fn noop4() align(4) void {} test "function alignment" { + // function alignment is a compile error on wasm32/wasm64 + if (builtin.arch == .wasm32 or builtin.arch == .wasm64) return error.SkipZigTest; + expect(derp() == 1234); expect(@TypeOf(noop1) == fn () align(1) void); expect(@TypeOf(noop4) == fn () align(4) void); @@ -117,6 +120,9 @@ fn sliceExpects4(slice: []align(4) u32) void { } test "implicitly decreasing fn alignment" { + // function alignment is a compile error on wasm32/wasm64 + if (builtin.arch == .wasm32 or builtin.arch == .wasm64) return error.SkipZigTest; + testImplicitlyDecreaseFnAlign(alignedSmall, 1234); testImplicitlyDecreaseFnAlign(alignedBig, 5678); } @@ -133,8 +139,8 @@ fn alignedBig() align(16) i32 { } test "@alignCast functions" { - // TODO investigate why this fails when cross-compiled to wasm. - if (builtin.os.tag == .wasi) return error.SkipZigTest; + // function alignment is a compile error on wasm32/wasm64 + if (builtin.arch == .wasm32 or builtin.arch == .wasm64) return error.SkipZigTest; expect(fnExpectsOnly1(simple4) == 0x19); } @@ -149,6 +155,9 @@ fn simple4() align(4) i32 { } test "generic function with align param" { + // function alignment is a compile error on wasm32/wasm64 + if (builtin.arch == .wasm32 or builtin.arch == .wasm64) return error.SkipZigTest; + expect(whyWouldYouEverDoThis(1) == 0x1); expect(whyWouldYouEverDoThis(4) == 0x1); expect(whyWouldYouEverDoThis(8) == 0x1); @@ -327,8 +336,8 @@ test "align(@alignOf(T)) T does not force resolution of T" { } test "align(N) on functions" { - // TODO investigate why this fails when cross-compiled to wasm. - if (builtin.os.tag == .wasi) return error.SkipZigTest; + // function alignment is a compile error on wasm32/wasm64 + if (builtin.arch == .wasm32 or builtin.arch == .wasm64) return error.SkipZigTest; expect((@ptrToInt(overaligned_fn) & (0x1000 - 1)) == 0); } From 08b0cae7778ea0946d595df451b9b4a65ff09abd Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Tue, 26 May 2020 18:00:08 +0200 Subject: [PATCH 2/2] Add matching compile error test --- test/compile_errors.zig | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/compile_errors.zig b/test/compile_errors.zig index b7c8053634..5fe42172ce 100644 --- a/test/compile_errors.zig +++ b/test/compile_errors.zig @@ -7445,4 +7445,20 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { , &[_][]const u8{ ":2:75: error: operation caused overflow", }); + + cases.addCase(x: { + var tc = cases.create("align(N) expr function pointers is a compile error", + \\export fn foo() align(1) void { + \\ return; + \\} + , &[_][]const u8{ + "tmp.zig:1:23: error: align(N) expr is not allowed on function prototypes in wasm32/wasm64", + }); + tc.target = std.zig.CrossTarget{ + .cpu_arch = .wasm32, + .os_tag = .freestanding, + .abi = .none, + }; + break :x tc; + }); }