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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D45711#1069483, @aheejin wrote:

> 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?


Indeed - but a bug is "the program behaves in a way that is not the intended behavior" - and if the behavior changes/is corrected, a test would be implemented to verify that the behavior has changed to reflect the intent, and to ensure that the behavior doesn't regress.

> `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 was 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()`.

OK, in the sense that it wouldn't crash - but not OK in the sense that it would produce the correct/intended answer, right? So could a test be implemented to verify that the answer is now correct, where it previously was incorrect? (previously the instruction ended up with a different (incorrect) debug location & now, with that change, it gets the correct location?)


Repository:
  rL LLVM

https://reviews.llvm.org/D45711