Skip to content
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.

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

Fix a fuzz issue with #6984 #6988

Merged
merged 6 commits into from
Oct 7, 2024
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Oct 4, 2024

When I refactored the optimizeDroppedBlock logic in #6982, I didn't move the
unreachability check with that code, which was wrong. When that function
was called from another place in #6984, the fuzzer found an issue.

Diff without whitespace is smaller.

The testcase is a little bizarre, but it's the one the fuzzer found and I can't
find a way to generate a better one (other than to reduce it, which I did).

@kripken kripken requested a review from aheejin October 4, 2024 23:41
@kripken
Copy link
Member Author

kripken commented Oct 4, 2024

Actually the test update here reverts almost all the test updates from #6984 - those changes were on blocks with unreachable children. The change was safe on them, but in general removing a block value in the presence of unreachable code is tricky, so it's best to avoid it.

;;
;; GUFA manages to generate IR that makes merge-blocks leave a stale type, if we
;; are not careful. simplify-locals-notee then errors if that stale type is
;; present, so all the passes are needed to hit this bizarre corner case.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this means... Why do all the passes need to hit this corner case wheh simplify-lcoals-notee errors out?

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't that the passes each need to do that, but that the testcase must include all those passes, for it to hit the error. I improved the text now.

@kripken kripken enabled auto-merge (squash) October 7, 2024 19:41
@kripken
Copy link
Member Author

kripken commented Oct 7, 2024

Hmm, the testcase now errors on CI for some reason I can't figure out. As it was a very low-value testcase anyhow (with that weird sequence of passes, it is hard to actually understand what's going wrong), I removed it.

@kripken kripken merged commit bcdedab into WebAssembly:main Oct 7, 2024
13 checks passed
@kripken kripken deleted the mb.unreachable branch October 7, 2024 20:54
kripken added a commit that referenced this pull request Oct 10, 2024
… too (#6994)

In #6984 we optimized dropped blocks even if they had unreachable code. In #6988
that part was reverted, and blocks with unreachable code were ignored once more.
However, I realized that the check was not actually for unreachable code, but for
having an unreachable child, so it would miss things like this:

(block
  (block
    ..
    (br $somewhere) ;; unreachable type, but no unreachable code
  )
)

But it is useful to merge such blocks: we don't need the inner block here.

To fix this, just run ReFinalize if we change anything, which will propagate
unreachability as needed. I think MergeBlocks was written before we had
that utility, so it didn't use it...

This is not only useful for itself but will unblock an EH optimization in a
later PR, that has code in this form. It also simplifies the code by removing
the hasUnreachableChild checks.
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.

2 participants