Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
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 [[ https://github.com/llvm-mirror/llvm/blob/a306456cafb95ea20023b82f407fefd2a9070213/lib/CodeGen/MachineBasicBlock.cpp#L1258-L1265 | 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).
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 [[ https://github.com/llvm-mirror/llvm/blob/a306456cafb95ea20023b82f407fefd2a9070213/lib/CodeGen/MachineBasicBlock.cpp#L1258-L1265 | 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?)
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 [[ https://github.com/llvm-mirror/llvm/blob/a306456cafb95ea20023b82f407fefd2a9070213/lib/CodeGen/MachineBasicBlock.cpp#L1258-L1265 | 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.
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?
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.
FYI, here is the code for [[ https://github.com/llvm-mirror/llvm/blob/21a0c18174343502c9f2b546a01333d1c351d9c0/include/llvm/CodeGen/MachineBasicBlock.h#L898-L903 | skipDebugInstructionsForward ]]. It only accesses the end iterator provided as the argument, and it is not a member function of MachineBasicBlock.