Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Oct 20, 2025

We already did this for Rethrow, and Try with delegate is almost the same.

@kripken kripken requested a review from aheejin October 20, 2025 20:57
@kripken
Copy link
Member Author

kripken commented Oct 20, 2025

Btw, I did this from memory - I wasn't sure where to find the spec for delegates, now it isn't in the proposal?

Comment on lines 2180 to 2182
if (i + 1 >= expressionStack.size()) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

I know it's preexisting, but is this necessary, given that we start i from expressionStack.size() - 2 above? If not, can we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I turned it into an assert.

if (curr->is<Rethrow>()) {
return child != tryy->body;
}
assert(curr->is<Try>());
Copy link
Member

Choose a reason for hiding this comment

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

Here curr (I guess accidentally) means the overwritten curr in line 2174, and because we have line 2175, isn't this always true?

while (1) {
  auto* curr = expressionStack[i];
  if (auto* tryy = curr->dynCast<Try>()) {
    ...
    assert(curr->is<Try>());

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was wrong, good catch. Fixed.

Index i = expressionStack.size() - 2;
// Rethrows and try-delegates must target a try. Find it.
while (1) {
auto* curr = expressionStack[i];
Copy link
Member

Choose a reason for hiding this comment

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

This curr now overwrites the function parameter curr, which can be confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, yeah, that looked wrong. I removed the loop var.

@kripken kripken merged commit 0f59f55 into WebAssembly:main Oct 21, 2025
16 checks passed
@kripken kripken deleted the try.del.fuzz branch October 21, 2025 17:43
@tlively
Copy link
Member

tlively commented Oct 21, 2025

Btw, I did this from memory - I wasn't sure where to find the spec for delegates, now it isn't in the proposal?

https://webassembly.github.io/spec/ in the "Legacy Extensions" section.

@kripken
Copy link
Member Author

kripken commented Oct 21, 2025

Thanks, good to know about the legacy section!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants