[lldb] Clear Frames when changing disable-language-runtime-unwindplans
#151208
Conversation
This patch uses the "setting changed" callback to clear previously
cached stack frames when
`target.process.disable-language-runtime-unwindplans` is changed. This
is necessary so that changing the setting followed by a `backtrace`
command produces an accurate backtrace.
With this, a user can create a custom command like below in order to
quickly inspect a backtrace created without language plugins.
```
debugger.HandleCommand("settings set target.process.disable-language-runtime-unwindplans true")
debugger.HandleCommand("bt " + command)
debugger.HandleCommand("settings set target.process.disable-language-runtime-unwindplans false")
```
In the future, we may consider implementing this as an option to
`backtrace`. Currently, this process setting is the only way of
affecting the unwinder, and changing the process setting as part of the
backtrace implementation doesn't feel right.
There are no upstream users of this flag, so we cannot write a test for
it here.
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThis patch uses the "setting changed" callback to clear previously cached stack frames when With this, a user can create a custom command like below in order to quickly inspect a backtrace created without language plugins. In the future, we may consider implementing this as an option to There are no upstream users of this flag, so we cannot write a test for it here. Full diff: https://github.com/llvm/llvm-project/pull/151208.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 7e66e315a867c..dc75d98acea70 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -100,6 +100,7 @@ class ProcessProperties : public Properties {
void SetStopOnSharedLibraryEvents(bool stop);
bool GetDisableLangRuntimeUnwindPlans() const;
void SetDisableLangRuntimeUnwindPlans(bool disable);
+ void DisableLanguageRuntimeUnwindPlansCallback();
bool GetDetachKeepsStopped() const;
void SetDetachKeepsStopped(bool keep_stopped);
bool GetWarningsOptimization() const;
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 2aa02fd58335e..385834795cb05 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -166,6 +166,9 @@ ProcessProperties::ProcessProperties(lldb_private::Process *process)
m_collection_sp->SetValueChangedCallback(
ePropertyPythonOSPluginPath,
[this] { m_process->LoadOperatingSystemPlugin(true); });
+ m_collection_sp->SetValueChangedCallback(
+ ePropertyDisableLangRuntimeUnwindPlans,
+ [this] { DisableLanguageRuntimeUnwindPlansCallback(); });
}
m_experimental_properties_up =
@@ -280,6 +283,13 @@ void ProcessProperties::SetDisableLangRuntimeUnwindPlans(bool disable) {
m_process->Flush();
}
+void ProcessProperties::DisableLanguageRuntimeUnwindPlansCallback() {
+ if (!m_process)
+ return;
+ for (auto thread_sp : m_process->Threads())
+ thread_sp->ClearStackFrames();
+}
+
bool ProcessProperties::GetDetachKeepsStopped() const {
const uint32_t idx = ePropertyDetachKeepsStopped;
return GetPropertyAtIndexAs<bool>(
|
| return; | ||
| for (auto thread_sp : m_process->Threads()) | ||
| thread_sp->ClearStackFrames(); | ||
| } |
There was a problem hiding this comment.
(In case you're curious: the Threads() method ensures the threads mutex is acquired for the scope of the range object)
|
This mostly looks fine, with a couple questions:
|
Based on the implementation of I did an experiment where I stopped a test program, selected
Do you mean something like: This seems to work too. What will obviously be bad is if a user starts a step-over operation, hits a breakpoint along the way, flips (but doesn't unflip!) this flag in a way that actually changes the backtrace, and then attempts to |
Thanks for doing these experiments... Given that it will change the nature of the stack when you change this variable, we should probably also clear the ThreadPlanStack for these threads when we clear the stack frames? Most plans will recognize that they no longer comprehend the new stack world and mark themselves Stale. But the chances of the thread plans still being valid over this change seems small, so it seems more stable to just clear them out. |
This is an interesting question, because if you toggle and then untoggle the setting, the thread plans should still work, as they keep track of StackIDs as opposed to StackFrames. If we were to clear the thread plans here, we would lose that ability. Likewise, if we were to clear the thread plans while stopped on a breakpoint hit during expression evaluation, what would be the consequences of this? Isn't expression evaluation driven by the thread plan? |
|
Another consideration: it is not the clearing of the stack frames that would confuse ThreadPlans, right? When we continue, the entire view of the stacks will be recomputed anyway. It is this recomputation done in a different way (through a different unwinder) that will confuse them, and not the clearing of the stack frames while execution was stopped |
|
The expression cancellation wouldn't be a problem, since you would pop the ThreadPlanCallExpression, which would take down the expression evaluation. It would cancel the evaluation, but that's probably what you want. The Thread plans generally expect to do work on stop, and on resume. They aren't expecting to have to re-evaluate the world in between those two points. So for instance they might have decided on stop that they needed to do some work which is no longer relevant because the stack view changed. But they really aren't expecting to have to make decisions on that till the next stop. The damage might have already been done by then. If your intention is to provide turning off the system unwinding as a "peek" operation, then you might consider stashing the thread plan stack to one side if the stack gets reinterpreted, and restoring it if they reverse the condition before the next "continue". Note, if we were being really exact, we'd love to only do this for thread that were changed by the language unwinder. Not sure if you know enough to determine that, however. |
|
Ok, you've convinced me. I've updated the PR to clean the thread plans.
Oh, interesting, I didn't know it would be clever enough to restore the state properly, but it does based on my testing!
While my intention is to provide a peek operation, I also want it to be relatively low-risk (and low effort). Stashing stack frames and restoring them seem a bit novel / risky, so I'm ok if we erase thread plans as a price of this; this process property breaks in-flight thread plans with or without this patch, so we're not changing the status quo with the callback. |
…ns` (llvm#151208) This patch uses the "setting changed" callback to clear previously cached stack frames when `target.process.disable-language-runtime-unwindplans` is changed. This is necessary so that changing the setting followed by a `backtrace` command produces an accurate backtrace. With this, a user can create a custom command like below in order to quickly inspect a backtrace created without language plugins. ``` debugger.HandleCommand("settings set target.process.disable-language-runtime-unwindplans true") debugger.HandleCommand("bt " + command) debugger.HandleCommand("settings set target.process.disable-language-runtime-unwindplans false") ``` In the future, we may consider implementing this as an option to `backtrace`. Currently, this process setting is the only way of affecting the unwinder, and changing the process setting as part of the backtrace implementation doesn't feel right. There are no upstream users of this flag, so we cannot write a test for it here. (cherry picked from commit 03bb10b)
…k_62 🍒 [lldb] Clear Frames when changing `disable-language-runtime-unwindplans` (llvm#151208)
This patch uses the "setting changed" callback to clear previously cached stack frames when
target.process.disable-language-runtime-unwindplansis changed. This is necessary so that changing the setting followed by abacktracecommand produces an accurate backtrace.With this, a user can create a custom command like below in order to quickly inspect a backtrace created without language plugins.
In the future, we may consider implementing this as an option to
backtrace. Currently, this process setting is the only way of affecting the unwinder, and changing the process setting as part of the backtrace implementation doesn't feel right.There are no upstream users of this flag, so we cannot write a test for it here.