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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Some nits.  Mostly I think the "idx" field in the frame list is redundant and only allows you to get the order wrong w/o adding useful functionality.

I haven't studied Pavel's changes to the Python lifecycle management yet, so I can't comment on whether that part of the patch is done correctly.

I don't think it's necessary at this point, but at some point you are going to have to make ScriptedStackFrame be a thing or there won't be a natural way to produce variables from the frame.  The other way to program it would be to fake a CFA, and then fake memory reads so that the regular Variable printing code will produce the ValueObjectVariable's.  I can see that being a useful method in some cases, but super-labor intensive in others...



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:151
+bool ScriptedThread::LoadArtificialStackFrames() {
+  StructuredData::ArraySP arr_sp = GetInterface()->GetStackFrames();
+
----------------
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.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:171
+
+    uint32_t frame_idx = GetStackFrameCount();
+    if (!dict->GetValueForKeyAsInteger("idx", frame_idx))
----------------
As I say below, I don't think the specified frame_idx is necessary.  But it's also odd that you initialize it to the total number of frames, then immediately overwrite is with the value from the dictionary.  If you don't get a value, you exit from this closure, so this seems wasted work.


================
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>(
----------------
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.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:273
   GetRegisterContext()->InvalidateIfNeeded(/*force=*/false);
+  LoadArtificialStackFrames();
 }
----------------
LoadArtificialStackFrames returns a bool for success/failure.  It always bugs me when returns signifying errors get dropped on the floor like this.


================
Comment at: lldb/test/API/functionalities/scripted_process/dummy_scripted_process.py:78
                 % (__name__, DummyScriptedProcess.__name__))
\ No newline at end of file

----------------
newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119388