[lldb] Add "settings modified" command

[kastiglione]

Adds settings modified which prints the settings which have been assigned in the current session. This command would be helpful for getting context about a debug session, to help explain debugger behavior.

rdar://157673186

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/152338.diff

3 Files Affected:

• (modified) lldb/include/lldb/Interpreter/OptionValueProperties.h (+3)
• (modified) lldb/source/Commands/CommandObjectSettings.cpp (+44)
• (modified) lldb/test/API/commands/settings/TestSettings.py (+11)

diff --git a/lldb/include/lldb/Interpreter/OptionValueProperties.h b/lldb/include/lldb/Interpreter/OptionValueProperties.h
index 91a3955962372..a68c233e189fd 100644
--- a/lldb/include/lldb/Interpreter/OptionValueProperties.h
+++ b/lldb/include/lldb/Interpreter/OptionValueProperties.h
@@ -163,6 +163,9 @@ class OptionValueProperties
     return false;
   }
 
+  auto begin() const { return m_properties.begin(); }
+  auto end() const { return m_properties.end(); }
+
 protected:
   Property *ProtectedGetPropertyAtIndex(size_t idx) {
     assert(idx < m_properties.size() && "invalid property index");
diff --git a/lldb/source/Commands/CommandObjectSettings.cpp b/lldb/source/Commands/CommandObjectSettings.cpp
index 7bbb0dd567ab1..5960feb8c4778 100644
--- a/lldb/source/Commands/CommandObjectSettings.cpp
+++ b/lldb/source/Commands/CommandObjectSettings.cpp
@@ -507,6 +507,48 @@ class CommandObjectSettingsList : public CommandObjectParsed {
   }
 };
 
+// CommandObjectSettingsModified -- List modified variables
+
+class CommandObjectSettingsModified : public CommandObjectParsed {
+public:
+  CommandObjectSettingsModified(CommandInterpreter &interpreter)
+      : CommandObjectParsed(interpreter, "settings modified",
+                            "List modified debugger settings.") {}
+
+  ~CommandObjectSettingsModified() override = default;
+
+protected:
+  void HandleProperties(OptionValueProperties *properties, Stream &strm) {
+    if (!properties)
+      return;
+
+    for (const auto &property : *properties) {
+      auto value_sp = property.GetValue();
+      if (!value_sp)
+        continue;
+
+      if (auto *subproperties = value_sp->GetAsProperties()) {
+        HandleProperties(subproperties, strm);
+        continue;
+      }
+
+      if (value_sp->OptionWasSet()) {
+        property.DumpQualifiedName(strm);
+        strm.PutCString(" = ");
+        value_sp->DumpValue(&m_exe_ctx, strm, OptionValue::eDumpOptionValue);
+        strm.EOL();
+      }
+    }
+  }
+
+  void DoExecute(Args &args, CommandReturnObject &result) override {
+    result.SetStatus(eReturnStatusSuccessFinishResult);
+
+    if (auto properties_sp = GetDebugger().GetValueProperties())
+      HandleProperties(properties_sp.get(), result.GetOutputStream());
+  }
+};
+
 // CommandObjectSettingsRemove
 
 class CommandObjectSettingsRemove : public CommandObjectRaw {
@@ -1070,6 +1112,8 @@ CommandObjectMultiwordSettings::CommandObjectMultiwordSettings(
                  CommandObjectSP(new CommandObjectSettingsShow(interpreter)));
   LoadSubCommand("list",
                  CommandObjectSP(new CommandObjectSettingsList(interpreter)));
+  LoadSubCommand("modified", CommandObjectSP(new CommandObjectSettingsModified(
+                                 interpreter)));
   LoadSubCommand("remove",
                  CommandObjectSP(new CommandObjectSettingsRemove(interpreter)));
   LoadSubCommand("replace", CommandObjectSP(
diff --git a/lldb/test/API/commands/settings/TestSettings.py b/lldb/test/API/commands/settings/TestSettings.py
index bc864942055c0..2dceb49e9eab8 100644
--- a/lldb/test/API/commands/settings/TestSettings.py
+++ b/lldb/test/API/commands/settings/TestSettings.py
@@ -1052,3 +1052,14 @@ def test_global_option(self):
         # NULL execution context (not one with an empty Target...) and in the
         # special handling for load-script-from-symbol-file this wasn't checked.
         self.runCmd("settings set -g target.load-script-from-symbol-file true")
+
+    def test_modified(self):
+        self.runCmd("settings set notify-void true")
+        self.runCmd("settings set target.process.optimization-warnings false")
+        self.expect(
+            "settings modified",
+            substrs=[
+                "notify-void = true",
+                "target.process.optimization-warnings = false",
+            ],
+        )

[jimingham]

All the other settings commands like settings list and settings show allow you to filter the info to the settings below a specified node. It would be useful here as well I think, and it's odd to have just this one not allow a filter.

[jimingham]

This is a useful command, BTW!

[Michael137]

Would this show settings modified but to their default values? Whatever the answer, lets add a test and document that case?

[DavidSpickett]

Would some (default: <value>) be useful? So I can see at a glance that crash-horribly = true (default: false) is what I should be investigating. A boolean is a bad example because ofc the default is the 1 other value but you see the idea.

[labath]

I am wondering if this wouldn't be better off as a flag on the "settings show" command: settings show --modified.

I can also imagine that, in the default mode, a plain "settings show" would render the modified settings differently (e.g., in bold)

[kastiglione]

@labath do you think it will be more discoverable that way, or more memorable?

[medismailben]

I'm with @DavidSpickett here: I think we should just show the original value next to the modified value:

(lldb) settings set use-color false
(lldb) settings show use-color
use-color (boolean) = false (default: true)

It makes discovery easier. We could also change the color of the settings whose value is different from the default, like we do for variables when their value change between runs. If we don't want to mess with the current output of settings show I think @labath approach with the --modified flag is also reasonable but less discoverable.

[labath]

I don't know about "discoverable", but I think it's more logical and fits it with the existing "noun verb" command naming scheme. You say you want to "show" "settings", and then you follow that with some modifiers/restrictions on that. Currently, the only restriction is via the setting path name (in a non-option argument), but I think an option would fit nicely there as well. Maybe that makes it more discoverable? I wouldn't have a problem finding it there...

The top level "settings" command doesn't have that many subcommands, so adding one more is not a big deal, but I also think that adding a flag makes the result more future-proof.

Now, if you want to change the way that settings are rendered in this mode (e.g. by including the default value), then a separate command may make more sense. Although, for default values specifically, I can also imagine this as being controlled by another command line flag (--verbose or something). And then you can combine the two however you see fit...

[jimingham]

I was looking for a reason to prefer one over the other. If we do this as a flag, then we could also do:

settings write -f foo.txt --modified target.process

or something like that to say "write all the modified settings under target.process".

So I guess I slightly lean towards the flag.

[kastiglione]

Thanks for all the feedback. The first step I've taken is to open a new PR which updates OptionValue subclasses to optionally print their default value. In that PR, the default can be seen using a new --verbose/-v flag to settings show #153233

Once that PR is accepted, I will update this PR to address the other suggestions/requests.