[PATCH] D79324: [WebAssembly] Fix block marker placing after fixUnwindMismatches

Mon May 4 19:23:04 PDT 2020

aheejin added a comment.

No it's complicated... Adobe's case triggered bug 1 (`placeBlockMarker` bug). That can be solvable by either applying bugfix 1 or  applying 2 and 3 together. Bugfix 3 means not placing `block`/`end_block` in that case bc it is not strictly necessary, and bugfix 1 means we place markers (even if it's unnecessary) that but correctly. But bugfix 3 requires bugfix 2 to work, because due to bug 2 `placeBlockMarker` does not find the 'header' in `placeBlockMarker` and just returns <https://github.com/llvm/llvm-project/blob/19f5da9c1d698653f942b504544a73b85b1e703c/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp#L237-L238>. The reason I put all 1~3 here is, even though the Adobe case can be fixed by 2 and 3, 1 is a good necessary caution too (actually it does the same check when it places `end_block` marker. It did not do that check when it placed `block`.)

Bugfix 4 is separate and is not triggered by the Adobe case, but I couldn't come up with a test case that triggers this because of the reason I stated in the CL description (it is valid code but is not generated by current wasm backend pipeline). The reason I included bugfix 4 here is 1. when I was fixing bug 3, which is we unnecessary place `block`/`end_block` marker sometimes, I accidentally found bug 4, which is the opposite - it does not place `block`/`end_block` when necessary, and 2. I can't come up with a separate test case for that anyway.

Our small test case here is not the same with Adobe case because it accidentally succeeds in the current code base, because bug 2 masks bug ... In the current codebase it tries to unnecessarily place block markers, but due to bug 2 it cannot find the header and doesn't end up triggering bug 1. but I couldn't come up with the test that has exactly the same properties with the Adobe one.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79324/new/

https://reviews.llvm.org/D79324