clayborg added a comment. In D58678#1410970 <https://reviews.llvm.org/D58678#1410970>, @jingham wrote:
> I have two questions about this patch. > > 1. I want some llvm expert to weigh in on whether > > m_instructions[i]->IsCall() > > always means it returns to the next instruction after the call. That seems > obvious, but since this patch depends on that being true, I'd like to know > that it is guaranteed. Yes, it would be good to know this > > > 2. The reason the test had to change (see Jonas' question) is because before > we would step over the breakpoint we were stopped at, then try to get to set > the next breakpoint, and when that fails we report the stopped step (since > the pc has moved) by presenting our usual stop notification. But the way the > branch search goes here, we fail before we do the step-over, and so the PC > hasn't moved, and so we don't do the stop listing. > > I'm not sure whether it is more confusing to get a stop notification when > the PC hasn't moved (albeit with an appropriate error) or whether it's more > confusing to have two ways the step error could be reported. I don't think there ever was a stop listing... This the "process status" that used to be in there! I believe the step would fail, and it wouldn't print anything, yet the PC had actually changed. If we did get a stop listing, then no "process status" would be needed? So I view this as an improvement over not getting anything and also the "thread step-over" used to claim it succeeded even though it failed. The only notification of this was from some tidbit in process status where the thread plan explanation claimed it failed. > This is a pretty minor issue and I really can't come down hard one way or the > other... If others don't have a strong opinion, its probably fine as is. So seems like there is another patch that might be better done by the stepping experts to clean up the "thread stepXXX" inconsistencies in stepping when errors happen during a step? I don't currently know enough about how and where this would best be done. Let me know what you think Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58678/new/ https://reviews.llvm.org/D58678 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
