Skip to content

[CFP][GSI] Fix catch pops after optimizing reads #9289

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
tlively merged 7 commits into from
Oct 29, 2026
+257 −74
Merged

[CFP][GSI] Fix catch pops after optimizing reads #9289

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
623a0bf
[CFP][GSI] Fix catch pops after optimizing reads
tlively Oct 20, 2026
2b05e0a
Merge remote-tracking branch 'origin/main' into cfp-block-nested-pops
tlively Oct 21, 2026
6f9cac0
replace getFirstPop, use PostWalker, depend on #9292
tlively Oct 22, 2026
6add684
Merge remote-tracking branch 'origin/main' into cfp-block-nested-pops
tlively Oct 28, 2026
f71a5ae
Remove unnecessary block-nested pop fixups and tests after #9292
tlively Oct 28, 2026
cfd5888
updates
tlively Oct 28, 2026
1056e85
comment
tlively Oct 29, 2026
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
174 changes: 100 additions & 74 deletions src/ir/eh-utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,80 +24,107 @@ namespace wasm {

namespace EHUtils {

// This returns three values, some of them as output parameters:
// - Return value: 'pop' expression (Expression*), when there is one in
// first-descendant line. If there's no such pop, it returns null.
// - isPopNested: Whether the discovered 'pop' is nested within a block
// - popPtr: 'pop' expression's pointer (Expression**), when there is one found
//
// When 'catchBody' itself is a 'pop', 'pop''s pointer is null, because there is
// no way to get the given expression's address. But that's fine because pop's
// pointer is only necessary (in handleBlockNestedPops) to fix it up when it is
// nested, and if 'catchBody' itself is a pop, we don't need to fix it up.
static Expression*
getFirstPop(Expression* catchBody, bool& isPopNested, Expression**& popPtr) {
Expression* firstChild = catchBody;
isPopNested = false;
popPtr = nullptr;

// Go down the line for the first child until we reach a leaf. A pop should be
// in that first-descendant line.
Expression** firstChildPtr = nullptr;
while (true) {
if (firstChild->is<Pop>()) {
popPtr = firstChildPtr;
return firstChild;
// Finds the first 'pop' in the given catch body that does not belong to an
// inner try's catch, and sets `isInvalid` to true if the pop is enclosed in a
// control flow structure (other than an unnamed block) or is not in the catch
// body's first-descendant line.
static Expression** findPopPtr(Expression*& catchBody,
bool& isInvalid,
bool stopIfInvalid = false) {
struct PopFinder : PostWalker<PopFinder> {
bool isInvalid = false;
bool stopIfInvalid = false;
bool pushedTask = false;
Expression** popPtr = nullptr;

PopFinder(Expression*& catchBody, bool stopIfInvalid)
: stopIfInvalid(stopIfInvalid) {
walk(catchBody);
}

void pushTask(TaskFunc func, Expression** currp) {
pushedTask = true;
PostWalker<PopFinder>::pushTask(func, currp);
}

void maybePushTask(TaskFunc func, Expression** currp) {
if (*currp) {
pushTask(func, currp);
}
}

if (Properties::isControlFlowStructure(firstChild)) {
if (auto* iff = firstChild->dynCast<If>()) {
// If's condition is a value child who comes before an 'if' instruction
// in binary, it is fine if a 'pop' is in there. We don't allow a 'pop'
// to be in an 'if''s then or else body because they are not first
// descendants.
firstChild = iff->condition;
firstChildPtr = &iff->condition;
continue;
} else if (firstChild->is<Loop>()) {
// We don't allow the pop to be included in a loop, because it cannot be
// run more than once
return nullptr;
static void scan(PopFinder* self, Expression** currp) {
if (self->popPtr || (self->stopIfInvalid && self->isInvalid)) {
return;
}

Expression* curr = *currp;
if (curr->is<Pop>()) {
self->popPtr = currp;
return;
}
if (auto* block = firstChild->dynCast<Block>()) {
// An unnamed block will be removed when written back. But if the block
// has a name,
// (catch $e
// (block $l0
// (pop i32) ;; within a block!
// (br $l0)
// ...
// )
// )
// It may be emitted in the binary format, so this is considered a
// nested pop (which we should fix).
if (block->name.is()) {
isPopNested = true;

if (auto* try_ = curr->dynCast<Try>()) {
// It is neither valid for a pop to appear inside a try body (or any
// other control flow structure that will be emitted in the binary), nor
// after the try.
self->isInvalid = true;

@kripken kripken Oct 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does a Try's existence make us invalid? The definition of invalid above is

if the pop is enclosed in a
control flow structure (other than an unnamed block) or is not in the catch
body's first-descendant line.

And I'm not sure how that connects to a Try's existence. For one thing, the Try may have no pop, for another, it may have a valid pop?

@tlively tlively Oct 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A catch block's pop cannot appear inside a try or any other control flow structure that will be emitted into the binary.

If isInvalid is not already set here, then we are still traversing down the path of leftmost children to find a valid pop. Instead, we have found a Try. If we find a pop after this, it is either inside the try body or not in the leftmost leaf position. Either way, it is invalid.

@kripken kripken Oct 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense, perhaps add a comment?

if (!self->stopIfInvalid) {
// Do not walk into inner catch bodies; pops in inner catch bodies
// belong to the inner catches.
self->pushTask(scan, &try_->body);
}
} else if (firstChild->is<Try>() || firstChild->is<TryTable>()) {
isPopNested = true;
} else {
WASM_UNREACHABLE("Unexpected control flow expression");
return;
}

if (!self->isInvalid && Properties::isControlFlowStructure(curr)) {
if (curr->is<If>()) {
// If's condition is a value child who comes before an 'if'
// instruction in binary, so it is fine if a 'pop' is in there. We
// don't allow a 'pop' to be in an 'if''s then or else body because
// they are not first descendants. PostWalker scans the condition
// first, and if the pop is not in the condition, reaching the end of
// the condition's first-descendant line will set `isInvalid` before
// the arms are scanned.
} else if (auto* block = curr->dynCast<Block>()) {
// Unnamed blocks are not emitted in the binary format, so a 'pop' at
// the beginning of an unnamed block is valid. Named blocks may be
// emitted if targeted by branches, so treat any 'pop' inside a named
// block as invalid.
if (block->name.is()) {
self->isInvalid = true;
}
} else {
assert(curr->is<Loop>() || curr->is<TryTable>());
self->isInvalid = true;
}
}

if (self->stopIfInvalid && self->isInvalid) {
return;
}

// If PostWalker::scan doesn't push a task to scan any children, then we
// must have reached a leaf node. The first leaf reached is the leftmost
// leaf, and the only valid place for a Pop to appear. We already know
// this is not a Pop, so if this is a leaf, then any later Pop will be
// invalid.
self->pushedTask = false;
PostWalker<PopFinder>::scan(self, currp);
if (!self->pushedTask) {
self->isInvalid = true;
}
}
ChildIterator it(firstChild);
if (it.getNumChildren() == 0) {
return nullptr;
}
firstChildPtr = &*it.begin();
firstChild = *firstChildPtr;
}
} finder(catchBody, stopIfInvalid);

isInvalid = finder.isInvalid;
return finder.popPtr;
}

bool containsValidDanglingPop(Expression* catchBody) {
bool isPopNested = false;
Expression** popPtr = nullptr;
auto* pop = getFirstPop(catchBody, isPopNested, popPtr);
return pop != nullptr && !isPopNested;
bool isInvalid = false;
auto* popPtr = findPopPtr(catchBody, isInvalid, /*stopIfInvalid=*/true);
return popPtr != nullptr && !isInvalid;
}

void handleBlockNestedPop(Try* try_, Function* func, Module& wasm) {
Expand All @@ -109,11 +136,10 @@ void handleBlockNestedPop(Try* try_, Function* func, Module& wasm) {
continue;
}

auto* catchBody = try_->catchBodies[i];
bool isPopNested = false;
Expression** popPtr = nullptr;
Expression* pop = getFirstPop(catchBody, isPopNested, popPtr);
assert(pop && "Pop has not been found in this catch");
auto*& catchBody = try_->catchBodies[i];
bool isInvalid = false;
Expression** popPtr = findPopPtr(catchBody, isInvalid);
assert(popPtr && "Pop has not been found in this catch");

// Change code like
// (catch $e
Expand All @@ -132,12 +158,12 @@ void handleBlockNestedPop(Try* try_, Function* func, Module& wasm) {
// (local.get $new)
// )
// )
if (isPopNested) {
assert(popPtr);
if (isInvalid) {
Expression* pop = *popPtr;
Index newLocal = builder.addVar(func, pop->type);
try_->catchBodies[i] =
builder.makeSequence(builder.makeLocalSet(newLocal, pop), catchBody);
*popPtr = builder.makeLocalGet(newLocal, pop->type);
catchBody =
builder.makeSequence(builder.makeLocalSet(newLocal, pop), catchBody);
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions src/passes/ConstantFieldPropagation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
#include <unordered_set>

#include "ir/bits.h"
#include "ir/eh-utils.h"
#include "ir/gc-type-utils.h"
#include "ir/possible-constant.h"
#include "ir/struct-utils.h"
Expand Down Expand Up @@ -447,6 +448,9 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
makeExpression(values[1 - testIndex].constant, refHeapType, curr)));

changed = true;
// Placing `ref` into the select condition (the third child) moves any
// enclosed `pop` out of the catch body's first-descendant line.
needEHFixups = true;
}

void doWalkFunction(Function* func) {
Expand All @@ -457,6 +461,9 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
if (changed) {
ReFinalize().walkFunctionInModule(func, getModule());
}
if (needEHFixups) {
EHUtils::handleBlockNestedPops(func, *getModule());
}
}

private:
Expand All @@ -466,6 +473,7 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
const bool refTest;

bool changed = false;
bool needEHFixups = false;
};

struct PCVScanner
Expand Down
8 changes: 8 additions & 0 deletions src/passes/GlobalStructInference.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@

#include "ir/bits.h"
#include "ir/debuginfo.h"
#include "ir/eh-utils.h"
#include "ir/find_all.h"
#include "ir/module-utils.h"
#include "ir/names.h"
Expand Down Expand Up @@ -353,6 +354,7 @@ struct GlobalStructInference : public Pass {
: parent(parent), globalsToUnnest(globalsToUnnest) {}

bool refinalize = false;
bool needEHFixups = false;

// As we prepare to un-nest globals, we create global.gets of the global
// that we will un-nest the content to. That global does not yet exist,
Expand Down Expand Up @@ -547,6 +549,9 @@ struct GlobalStructInference : public Pass {
builder.makeRefEq(builder.makeRefAs(RefAsNonNull, ref), getGlobal),
left,
right));
// Placing `ref` into the select condition (the third child) moves any
// enclosed `pop` out of the catch body's first-descendant line.
needEHFixups = true;
}

void visitRefCast(RefCast* curr) {
Expand Down Expand Up @@ -603,6 +608,9 @@ struct GlobalStructInference : public Pass {
if (refinalize) {
ReFinalize().walkFunctionInModule(func, getModule());
}
if (needEHFixups) {
EHUtils::handleBlockNestedPops(func, *getModule());
}
}

Value readFromStructNew(StructNew* structNew,
Expand Down
83 changes: 83 additions & 0 deletions test/lit/passes/cfp-reftest.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1669,3 +1669,86 @@
)
)
)

(module
(rec
;; CHECK: (rec
;; CHECK-NEXT: (type $super (sub (struct (field i32))))
(type $super (sub (struct (field i32))))
;; CHECK: (type $sub1 (sub final $super (struct (field i32))))
(type $sub1 (sub final $super (struct (field i32))))
;; CHECK: (type $sub2 (sub final $super (struct (field i32) (field i64))))
(type $sub2 (sub final $super (struct (field i32) (field i64))))
)
;; CHECK: (type $3 (func))

;; CHECK: (type $4 (func (param (ref $super))))

;; CHECK: (tag $e (type $4) (param (ref $super)))
(tag $e (param (ref $super)))

;; CHECK: (func $make (type $3)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $sub1
;; CHECK-NEXT: (i32.const 10)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.new $sub2
;; CHECK-NEXT: (i32.const 20)
;; CHECK-NEXT: (i64.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $make
(drop
(struct.new $sub1
(i32.const 10)
)
)
(drop
(struct.new $sub2
(i32.const 20)
(i64.const 0)
)
)
)

;; CHECK: (func $catch-pop (type $3)
;; CHECK-NEXT: (local $0 (ref $super))
;; CHECK-NEXT: (try
;; CHECK-NEXT: (do
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (pop (ref $super))
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (select
;; CHECK-NEXT: (i32.const 10)
;; CHECK-NEXT: (i32.const 20)
;; CHECK-NEXT: (ref.test (ref $sub1)
;; CHECK-NEXT: (ref.as_non_null
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $catch-pop
;; Optimizing this struct.get into a select moves its reference into the
;; select condition (third child), requiring the enclosed pop to be fixed up.
(try
(do)
(catch $e
(drop
(struct.get $super 0
(pop (ref $super))
)
)
)
)
)
)
Loading
Loading