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