[Lldb-commits] [PATCH] D150236: Thread::GetStackFrameCount should forestall interruption

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
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);
----------------
Is the range inclusive of `end_idx`? as in, is it [0, end_idx) or [0, end_idx]?


================
Comment at: lldb/source/Target/StackFrameList.cpp:89
 
-  GetFramesUpTo(0);
+  GetFramesUpTo(0, false);
   if (m_frames.empty())
----------------
nit:
```
GetFramesUpTo(/*end_idx = */0, /*allow_interrupt = */false);
```

Or something like this... A little easier to understand IMO.


================
Comment at: lldb/source/Target/StackFrameList.cpp:640-641
   if (can_create)
-    GetFramesUpTo(UINT32_MAX);
+    GetFramesUpTo(UINT32_MAX, false); // Don't allow interrupt or we might not
+                                      // return the correct count
 
----------------
Same here I think.


================
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);
----------------
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.


================
Comment at: lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py:16
+    def test_backtrace_interrupt(self):
+        """There can be many tests in a test case - describe this test here."""
+        self.build()
----------------
I have a feeling this docstring was supposed to be different?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150236/new/

https://reviews.llvm.org/D150236