Page MenuHomePhabricator

[WebAssembly] Add an assertion for an invalid CFG
ClosedPublic

Authored by aheejin on Apr 16 2018, 5:35 PM.

Details

Summary

It was not easy to provide a test case for D45648 (rL330079) because the bug
didn't manifest itself in the set of currently valid IRs. Added an assertion to
check this faster, thanks to @dblaikie's suggestion.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Apr 16 2018, 5:35 PM

So does this make the other change NFC (under this assertion)? (ie: if the assertion is true, is "MBB" and "*Header" equivalent/the same thing?)

We can keep the other change even if this is true, if it's a nicer way of expressing the code. But I'm trying to ensure that there's not a semantic change hidden in here that would need testing. If all the things with semantic changes would trigger the assertion first/instead, then that's fine.

aheejin added a comment.EditedApr 16 2018, 7:13 PM

I don't understand. The other change (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. 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 (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 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).

I don't understand. The other change (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. 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 (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 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?)

aheejin added a comment.EditedApr 16 2018, 7:22 PM

No, it was producing the correct answer. As long as Header was not empty, whether that part of the code was Header or MBB didn't affect the result of the code. That's what I said in the mail. Have a look at MachineBasicBlock::findDebugLoc. So I couldn't come up with a valid test case that was failing under the buggy code bug does not fail anymore now.

No, it was producing the correct answer. As long as Header was not empty, whether that part of the code was Header or MBB didn't affect the result of the code.

Why wouldn't that effect the result of the code? Header and MBB would have different instructions that could have different debug locations, right? So the code would produce different debug locations for the resulting instruction depending on whether it used Header on MBB?

Oh, I see what you mean - findDebugLoc doesn't actually use any of the members of the MachineBasicBlock it's called on (well, it uses the instr_end - so this code was invoking undefined behavior, presumably, when it passed an instruction iterator to an MBB that the instruction wasn't part of?)

So it got the right answer, despite the undefined behavior, except in the case where InsertPos was == end. So it sort of answers my original question (I phrased it poorly, because I assumed the implementation of findDebugLoc depended on the instance it was called on - which it should/dose, but not in a particularly meaningful way - only in a "invokes UB but probably is fine" kind of way)

Maybe this could be tested for, though - without debug info this findDebugLoc would potentially search forever or crash, perhaps? Since it would never compare == end (because it's a comparison across two different lists)? Seems like it'd be good to have that test case?

aheejin added a comment.EditedApr 16 2018, 7:49 PM

Maybe this could be tested for, though - without debug info this findDebugLoc would potentially search forever or crash, perhaps? Since it would never compare == end (because it's a comparison across two different lists)? Seems like it'd be good to have that test case?

No, if there is at least a single instruction within Header and it does not have debug info, it would just return its empty debug info. It wouldn't search onward past end().

OK - so would "skipDebugInstructionsForward" ever need the end iterator?
since it should always come across a non-debug instruction (all valid BBs
have at least one non-debug instruction, the terminator - though I guess
you might call this function on an invalid basic block during
construction/intermediate state changes, etc).

& if it doesn't need the end, then it doesn't need the MBB and could be a
static function.

dblaikie accepted this revision.Apr 17 2018, 12:23 PM

ANyway, seems OK.

This revision is now accepted and ready to land.Apr 17 2018, 12:23 PM

FYI, here is the code for skipDebugInstructionsForward. It only accesses the end iterator provided as the argument, and it is not a member function of MachineBasicBlock.

This revision was automatically updated to reflect the committed changes.