[PATCH] D45711: [WebAssembly] Add an assertion for an invalid CFG

Heejin Ahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 16 19:13:27 PDT 2018


aheejin added a comment.

I don't understand. The other change (https://reviews.llvm.org/D45648) was a bugfix, and `MBB` and `Header` are not the same thing, which was the reason I fixed that bug. A bugfix is not supposed to be NFC, right?

`InsertPos` here is computed by this snippet of code <https://github.com/llvm-mirror/llvm/blob/fddfe33105e327c8fe487bfe3a39e28e9ed2f154/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp#L130-L147>. As you can see, there's no way that `InsertPos` is somewhere in `MBB`; `InsertPos` should point some instruction within `Header`, or be equal to `Header->end()`, because it was originated from `Header`, not `MBB`. So this is what I meant by that bugfix (https://reviews.llvm.org/D45648) was trivial enough, almost to the extent of a typo fix. The reason the previous buggy code wasm running OK was, as you can see in `MachineBasicBlock::findDebugLoc` <https://github.com/llvm-mirror/llvm/blob/a306456cafb95ea20023b82f407fefd2a9070213/lib/CodeGen/MachineBasicBlock.cpp#L1258-L1265> function, it's OK as long as the BB where the iterator `MBBI` is in is not empty, because in that way it wouldn't hit `instr_end()`.

So in short, in this function, `MBB` and `Header` are two different blocks and there is no case they can be the same. The previous buggy code just happened to run OK, because `Header` was always not empty (it always has a terminator, because that's the precondition to enter this part of the code).


Repository:
  rL LLVM

https://reviews.llvm.org/D45711





More information about the llvm-commits mailing list