Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,16 @@ There are a few differences between Binaryen IR and the WebAssembly language:
much about this when writing Binaryen passes. For more details see the
`requiresNonNullableLocalFixups()` hook in `pass.h` and the
`LocalStructuralDominance` class.
* Binaryen IR uses the most refined types possible for references,
specifically:
* The IR type of a `ref.func` is always a specific function type, and not
plain `funcref`. It is also non-nullable.
* Non-nullable types are also used for the type that `try_table` sends
on branches (if we branch, a null is never sent), that is, it sends
(ref exn) and not (ref null exn).
In both cases if GC is not enabled then we emit the less-refined type in the
binary. When reading a binary, the more refined types will be applied as we
build the IR.
* `br_if` output types are more refined in Binaryen IR: they have the type of
the value, when a value flows in. In the wasm spec the type is that of the
branch target, which may be less refined. Using the more refined type here
Expand Down
8 changes: 5 additions & 3 deletions src/wasm/wasm-binary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1544,9 +1544,9 @@ void WasmBinaryWriter::writeInlineBuffer(const char* data, size_t size) {

void WasmBinaryWriter::writeType(Type type) {
if (type.isRef()) {
// The only reference types allowed without GC are funcref and externref. We
// internally use more refined versions of those types, but we cannot emit
// those more refined types.
// The only reference types allowed without GC are funcref, externref, and
// exnref. We internally use more refined versions of those types, but we
// cannot emit those without GC.
if (!wasm->features.hasGC()) {
auto ht = type.getHeapType();
if (ht.isMaybeShared(HeapType::string)) {
Expand All @@ -1555,6 +1555,8 @@ void WasmBinaryWriter::writeType(Type type) {
// string, the stringref feature must be enabled.
type = Type(HeapTypes::string.getBasic(ht.getShared()), Nullable);
} else {
// Only the top type (func, extern, exn) is available, and only the
// nullable version.
type = Type(type.getHeapType().getTop(), Nullable);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/wasm/wasm-validator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2658,7 +2658,7 @@ void FunctionValidator::visitTryTable(TryTable* curr) {
"the number of catch tags and sent types do not match");

const char* invalidSentTypeMsg = "invalid catch sent type information";
Type exnref = Type(HeapType::exn, Nullable);
Type exnref = Type(HeapType::exn, NonNullable);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it's better to make types as refined as possible for optimization, but shouldn't we not error error out on nullable exnref in the validator? I think we also generate nullable exnrefs in the fuzzer. I think we should leave it as is and check for not the equality but the subtype below (in line 2696)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be sending a non-nullable exnref. The target might be less refined (until ReFinalize fixes that) but we should still be sending the refined type, as the spec says?

Unless I missed something in the code here and it is checking something other than the type being sent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm misunderstanding you, can you give an example of code that should validate but this code will not accept?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might have misunderstood what this PR does. So basically, from now on we assume exnref == (ref exn) and not (ref null exn) and treat it as such in the IR and all passes, and make it (ref null exn) only when we write it back to a binary. Right? (By the way don't we need to do the same thing when we write it back as a text?)

If that's the case, if you programatically create a TryTable object with exnref in sentType field, it will not validate. Of course, we don't usually manually set the fields in the IR classes; we usually use constructors and finalize methods, so I guess practically it's fine, but it was a little confusing to me. (Maybe giving TryTable::sentType a private visibility might help a little.)

I think it'd help if we have little more comments in wasm.cpp that why we are doing this. Also, can't we also validate (ref null exn) in sentType as valid? (I can't give you an example wast program that does not validate, because we basically read exnref as (ref exn) when reading.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exnref should be equivalent to (ref null exn), not (ref exn). Since try_table never produces a null exn value, the most precise type for the exn value it produces is always (ref exn). We can therefore take it as an invariant that the IR models sending a (ref exn). However, non-nullable references like that do not exist without GC enabled, so without GC, we fix it up by transforming the (ref exn) into exnref (i.e. (ref null exn)) in the binary writer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment now. I also noticed we were missing docs on this in the readme and added those.

for (Index i = 0; i < curr->catchTags.size(); i++) {
auto sentType = curr->sentTypes[i];
size_t tagTypeSize;
Expand Down
6 changes: 5 additions & 1 deletion src/wasm/wasm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,11 @@ static void populateTryTableSentTypes(TryTable* curr, Module* wasm) {
return;
}
curr->sentTypes.clear();
Type exnref = Type(HeapType::exn, Nullable);
// We always use the refined non-nullable type in our IR, which is what the
// wasm spec defines when GC is enabled (=== non-nullable types are allowed).
// If GC is not enabled then we emit a nullable type in the binary format in
// WasmBinaryWriter::writeType.
Type exnref = Type(HeapType::exn, NonNullable);
for (Index i = 0; i < curr->catchTags.size(); i++) {
auto tagName = curr->catchTags[i];
std::vector<Type> sentType;
Expand Down
27 changes: 27 additions & 0 deletions test/lit/basic/exception-handling-no-gc.wast
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.

;; Test that we do not emit an invalid (ref exn) when exceptions are enabled
;; but not GC. GC is required for us to be non-nullable.

;; RUN: wasm-opt %s --enable-reference-types --enable-exception-handling --disable-gc --roundtrip -S -o - | filecheck %s

(module
;; CHECK: (func $test (result exnref)
;; CHECK-NEXT: (block $label$1 (result exnref)
;; CHECK-NEXT: (try_table (catch_all_ref $label$1)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test (result exnref)
;; It is valid to write (ref exn) in Binaryen IR, and internally that is how
;; we represent things, but when we emit the binary we emit a nullable type,
;; so after the roundtrip we are less refined.
(block $label (result (ref exn))
(try_table (catch_all_ref $label)
(unreachable)
)
)
)
)

2 changes: 1 addition & 1 deletion test/lit/passes/merge-blocks-eh.wast
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@

;; CHECK: (func $drop-block-try_catch_multi_partial (type $0)
;; CHECK-NEXT: (tuple.drop 2
;; CHECK-NEXT: (block $outer (type $1) (result i32 exnref)
;; CHECK-NEXT: (block $outer (type $2) (result i32 (ref exn))
;; CHECK-NEXT: (block $inner
;; CHECK-NEXT: (try_table (catch_ref $i32 $outer) (catch_all $inner)
;; CHECK-NEXT: (call $import)
Expand Down
Loading
Loading