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

[Med Ismail Bennani via Phabricator via lldb-commits]

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