[Lldb-commits] [PATCH] D119388: [lldb/Plugin] Add artificial stackframe loading in ScriptedThread

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 4 inline comments as done.
mib added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:151
+bool ScriptedThread::LoadArtificialStackFrames() {
+  StructuredData::ArraySP arr_sp = GetInterface()->GetStackFrames();
+
----------------
jingham wrote:
> The GetStackFrames call should take a Status parameter, in case it has some good reason why it couldn't fetch the stack frames.  It will make debugging these things a lot easier if you can pass the real error back to yourself.
For now the Scripted{Process,Thread}Interface will log the error to the appropriate channel, but I can see having all the interface methods return an `llvm::Expected<T>` object with an error message instead. I'll do it in a follow-up patch. 


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:172
+    uint32_t frame_idx = GetStackFrameCount();
+    if (!dict->GetValueForKeyAsInteger("idx", frame_idx))
+      return ScriptedInterface::ErrorWithMessage<bool>(
----------------
jingham wrote:
> It seems awkward to have the producer of artificial stack frames number the frames.  After all, they are already putting them into an array, and the natural way to do that is in stack frame order.  Unless you see some really good use case for putting them in scrambled and using the "idx" key to unscramble them, the redundant idx seems like something you could get wrong w/o adding much value.
Makes sense.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:273
   GetRegisterContext()->InvalidateIfNeeded(/*force=*/false);
+  LoadArtificialStackFrames();
 }
----------------
jingham wrote:
> LoadArtificialStackFrames returns a bool for success/failure.  It always bugs me when returns signifying errors get dropped on the floor like this.
`Thread::RefreshStateAfterStop` doesn't return anything, and the failure case is already handled in `LoadArtificialStackFrames` by logging an error message in the appropriate channel.

Do you have any other suggestion on how I could improve this ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119388