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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
