Skip to content

[GC] Fix TypeRefining on StructGets without content but with a reachable ref #7138

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 9, 2024
Merged

[GC] Fix TypeRefining on StructGets without content but with a reachable ref #7138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
115d9f7
fix
kripken Dec 5, 2024
11bfd28
tests
kripken Dec 5, 2024
79752b9
oops
kripken Dec 5, 2024
8f9f8e8
fix
kripken Dec 5, 2024
b680a3f
format
kripken Dec 5, 2024
2af2c59
feedbac
kripken Dec 9, 2024
63e023c
fix
kripken Dec 9, 2024
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
104 changes: 42 additions & 62 deletions src/passes/TypeRefining.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,13 @@ struct FieldInfoScanner

void
noteDefault(Type fieldType, HeapType type, Index index, FieldInfo& info) {
// Default values do not affect what the heap type of a field can be turned
// into. Note them, however, as they force us to keep the type nullable.
// Default values must be noted, so that we know there is content there.
if (fieldType.isRef()) {
info.note(Type(fieldType.getHeapType().getBottom(), Nullable));
// All we need to note here is nullability (the field must remain
// nullable), but not anything else about the type.
fieldType = Type(fieldType.getHeapType().getBottom(), Nullable);
}
info.note(fieldType);
}

void noteCopy(HeapType type, Index index, FieldInfo& info) {
Expand Down Expand Up @@ -270,71 +272,49 @@ struct TypeRefining : public Pass {
return;
}

if (curr->ref->type.isNull()) {
// This get will trap. In theory we could leave this for later
// optimizations to do, but we must actually handle it here, because
// of the situation where this get's type is refined, and the input
// type is the result of a refining:
//
// (struct.get $A ;; should be refined to something
// (struct.get $B ;; just refined to nullref
//
// If the input has become a nullref then we can't just return out of
// this function, as we'd be leaving a struct.get of $A with the
// wrong type. But we can't find the right type since in Binaryen IR
// we use the ref's type to see what is being read, and that just
// turned into nullref. To avoid that corner case, just turn this code
// into unreachable code now, and the later refinalize will turn all
// the parents unreachable, avoiding any type-checking problems.
Builder builder(*getModule());
replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
builder.makeUnreachable()));
return;
Type newFieldType;
if (!curr->ref->type.isNull()) {
auto oldType = curr->ref->type.getHeapType();
newFieldType = parent.finalInfos[oldType][curr->index].getLUB();
}

auto oldType = curr->ref->type.getHeapType();
auto newFieldType = parent.finalInfos[oldType][curr->index].getLUB();
if (Type::isSubType(newFieldType, curr->type)) {
// This is the normal situation, where the new type is a refinement of
// the old type. Apply that type so that the type of the struct.get
// matches what is in the refined field. ReFinalize will later
// propagate this to parents.
//
// Note that ReFinalize will also apply the type of the field itself
// to a struct.get, so our doing it here in this pass is usually
// redundant. But ReFinalize also updates other types while doing so,
// which can cause a problem:
//
// (struct.get $A
// (block (result (ref null $A))
// (ref.null any)
// )
// )
//
// Here ReFinalize will turn the block's result into a bottom type,
// which means it won't know a type for the struct.get at that point.
// Doing it in this pass avoids that issue, as we have all the
// necessary information. (ReFinalize will still get into the
// situation where it doesn't know how to update the type of the
// struct.get, but it will just leave the existing type - it assumes
// no update is needed - which will be correct, since we've updated it
// ourselves here, before.)
curr->type = newFieldType;
} else {
// This instruction is invalid, so it must be the result of the
// situation described above: we ignored the read during our
// inference, and optimized accordingly, and so now we must remove it
// to keep the module validating. It doesn't matter what we emit here,
// since there are no struct.new or struct.sets for this type, so this
// code is logically unreachable.
//
// Note that we emit an unreachable here, which changes the type, and
// so we should refinalize. However, we will be refinalizing later
// anyhow in updateTypes, so there is no need.
if (curr->ref->type.isNull() || newFieldType == Type::unreachable ||
!Type::isSubType(newFieldType, curr->type)) {
Copy link
Member

@tlively tlively Dec 6, 2024

Choose a reason for hiding this comment

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

Do we have a test that tickles this last !Type::isSubType(...) condition? It is surprising that that could ever happen.

Copy link
Member Author

@kripken kripken Dec 9, 2024

Choose a reason for hiding this comment

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

Here is the relevant test:

binaryen/test/lit/passes/type-refining.wast

Line 802 in 729ea41

(module

The test hits an internal assertion without this condition.

// This get will trap, or cannot be reached: either the ref is null,
// or the field is never written any contents, or the contents we see
// are invalid (they passed through some fallthrough that will trap at
// runtime). Emit unreachable code here.
Builder builder(*getModule());
replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
builder.makeUnreachable()));
return;
}

// This is the normal situation, where the new type is a refinement of
// the old type. Apply that type so that the type of the struct.get
// matches what is in the refined field. ReFinalize will later
// propagate this to parents.
//
// Note that ReFinalize will also apply the type of the field itself
// to a struct.get, so our doing it here in this pass is usually
// redundant. But ReFinalize also updates other types while doing so,
// which can cause a problem:
//
// (struct.get $A
// (block (result (ref null $A))
// (ref.null any)
// )
// )
//
// Here ReFinalize will turn the block's result into a bottom type,
// which means it won't know a type for the struct.get at that point.
// Doing it in this pass avoids that issue, as we have all the
// necessary information. (ReFinalize will still get into the
// situation where it doesn't know how to update the type of the
// struct.get, but it will just leave the existing type - it assumes
// no update is needed - which will be correct, since we've updated it
// ourselves here, before.)
curr->type = newFieldType;
}
};

Expand Down
130 changes: 130 additions & 0 deletions test/lit/passes/type-refining.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1574,3 +1574,133 @@
(ref.null $8)
)
)

;; Test for a bug where we made a struct.get unreachable because it was reading
;; a field that had no writes, but in a situation where it is invalid for the
;; struct.get to be unreachable.
(module
;; CHECK: (type $never (sub (struct (field i32))))
(type $never (sub (struct (field i32))))

;; CHECK: (rec
;; CHECK-NEXT: (type $optimizable (struct (field (mut nullfuncref))))
(type $optimizable (struct (field (mut (ref null func)))))

;; CHECK: (type $2 (func))

;; CHECK: (global $never (ref null $never) (ref.null none))
(global $never (export "never") (ref null $never)
;; Make the type $never public (if it were private, we'd optimize in a
;; different way that avoids the bug that this tests for).
(ref.null $never)
)

;; CHECK: (export "never" (global $never))

;; CHECK: (func $setup (type $2)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $optimizable
;; CHECK-NEXT: (ref.null nofunc)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result (ref none))
;; CHECK-NEXT: (ref.as_non_null
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $setup
;; A struct.new, so that we have a field to refine (which avoids the pass
;; early-exiting).
(drop
(struct.new $optimizable
(ref.null func)
)
)
;; A struct.get that reads a $never, but the actual fallthrough value is none.
;; We never create this type, so the field has no possible content, and we can
;; replace this with an unreachable.
(drop
(struct.get $never 0
(block (result (ref $never))
(ref.as_non_null
(ref.null none)
)
)
)
)
)
)

;; Test that we note default values.
(module
(rec
;; CHECK: (rec
;; CHECK-NEXT: (type $A (sub (struct (field i32))))
(type $A (sub (struct (field i32))))
;; CHECK: (type $B (sub (struct (field i32))))
(type $B (sub (struct (field i32))))
)
;; CHECK: (type $2 (func (param (ref null $A) (ref null $B))))

;; CHECK: (type $optimizable (sub (struct (field (ref $2)))))
(type $optimizable (sub (struct (field funcref))))

;; CHECK: (elem declare func $test)

;; CHECK: (export "test" (func $test))

;; CHECK: (func $test (type $2) (param $x (ref null $A)) (param $y (ref null $B))
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $optimizable
;; CHECK-NEXT: (ref.func $test)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get $A 0
;; CHECK-NEXT: (struct.new $A
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get $B 0
;; CHECK-NEXT: (struct.new_default $B)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test (export "test") (param $x (ref null $A)) (param $y (ref null $B))
;; Use $A, $B as params of this export, so they are public.

;; Make something for the pass to do, to avoid early-exit.
(drop
(struct.new $optimizable
(ref.func $test)
)
)

;; Get from a struct.new. We have nothing to optimize here. (In particular, we
;; cannot make this unreachable, as there is a value in the field, 0.)
(drop
(struct.get $A 0
(struct.new $A
(i32.const 0)
)
)
)

;; As above. Now the value in the field comes from a default value.
(drop
(struct.get $B 0
(struct.new_default $B)
)
)
)
)
Loading