mib requested changes to this revision.
mib added a comment.
This revision now requires changes to proceed.
I don't understand why the succeeding return value for `GetFramesUpTo` is
`false`. It looks counter-intuitive to me. What's the motivation behind that ?
================
Comment at: lldb/include/lldb/Target/StackFrameList.h:103-104
- void GetFramesUpTo(uint32_t end_idx);
+ /// Gets frames up to end_idx (which can be greater than the actual number of
+ /// frames.) Returns true if the function was interrupted, false otherwise.
+ bool GetFramesUpTo(uint32_t end_idx, bool allow_interrupt = true);
----------------
bulbazord wrote:
> Is the range inclusive of `end_idx`? as in, is it [0, end_idx) or [0,
> end_idx]?
@bulbazord I believe this should be inclusive.
@jingham This comment sounds like we only return `true` if the function was
interrupted, which is not the expected behavior, right ?
================
Comment at: lldb/source/Target/StackFrameList.cpp:448
if (m_frames.size() > end_idx || GetAllFramesFetched())
- return;
+ return false;
----------------
I find it confusing the fail here given that we've succeeded at fetching the
frames
================
Comment at: lldb/source/Target/StackFrameList.cpp:633
+ return was_interrupted;
+ return false;
}
----------------
Again, I believe this should return `true` since we succeeded at retrieving all
the requested frames.
================
Comment at: lldb/source/Target/StackFrameList.cpp:683-684
// there are that many. If there weren't then you asked for too many frames.
- GetFramesUpTo(idx);
+ bool interrupted = GetFramesUpTo(idx);
+ if (interrupted) {
+ Log *log = GetLog(LLDBLog::Thread);
----------------
bulbazord wrote:
> nit: Since `interrupted` isn't used anywhere else, you could do something
> like:
> ```
> if (bool interrupted = GetFramesUpTo(idx)) {
> // Code here
> }
> ```
> You could also just do `if (GetFramesUpTo(idx))` but I think the name of the
> function isn't descriptive enough to do that and stay readable.
+1: reading `if (GetFramesUpTo(idx))`, I'd never think that the succeeding
result would be `false`.
================
Comment at:
lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py:46
+
+ self.dbg.CancelInterruptRequest()
+
----------------
Is this necessary if you already have it in the `cleanup` method ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150236/new/
https://reviews.llvm.org/D150236
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
