Clear all settings during a test's setUp

[tatyana-krasnukha]

Summary

Moved from D74903

[labath]

Clearing all settings is a reasonable operation, so I don't have any problem with it per se. I'm just wondering, given that this is not something that one would want to do very often (and it can have catastrophic results), whether this operation should not be more explicit. "settings clear --all" for instance ? Adding @jingham for the interface aspects.

It would also be nice to add a test case for the new command.

[tatyana-krasnukha]

Added --all property to settings clear + added a test

[tatyana-krasnukha]

Removed TestFixIts.py as those changes relate to the parent revision.

[labath]

lldb/source/Commands/CommandObjectSettings.cpp

1119–1122

What will happen if I pass both --all and an actual setting argument. I think that should be an error.

lldb/test/API/commands/settings/TestSettings.py

543–546

This dependence on setUpCommands looks weird. Any way we can get rid of that? Maybe if we check the exact four settings that you modified before clear --all and verifying they have the correct (hardcoded) value? (and maybe not use the term-width setting, since the default value of that is unpredictable)

[tatyana-krasnukha]

Addressed comments

[labath]

Awesome, thanks.

[teemperor]

Somehow this broke the macOS LLDB bot:

Failing Tests (2):
    lldb-api :: functionalities/inline-stepping/TestInlineStepping.py
    lldb-api :: lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py

[teemperor]

I guess now all the shady hacks are coming out. The following settings change their value after calling settings clear after startup in the test:

target.process.thread.step-avoid-regexp (regex) -> from '^std::' to empty string
platform.module-cache-directory (file) -> from '"/Users/teemperor/.lldb/module_cache"' to empty string
script-lang (enum) -> from 'default' to 'python'

[tatyana-krasnukha]

Thank you for details, I'm looking at these failures, however, I'm not able to debug on the macOS.

[teemperor]

Sure, I can take care of it. It seems OptionValueRegex::Clear is always resetting to an empty regex. I guess the same goes for the other option values that are failing.

[teemperor]

Fixed in 352f16db87f583ec7f55f8028647b5fd8616111f

[tatyana-krasnukha]

platform.module-cache-directory should be fixed by rGfe74df01a909.

Regarding script-lang, its default value is eScriptLanguagePython in CoreProperties.td and I'm not sure that I should change it.

lldb/packages/Python/lldbsuite/test/lldbtest.py

class Base(unittest2.TestCase):
 
     def getSourcePath(self, name):
         """Return absolute path to a file in the test's source directory."""
         return os.path.join(self.getSourceDir(), name)
 
     @classmethod
     def setUpCommands(cls):
         commands = [
+            # First of all, clear all settings to have clean state of global properties.
+            "settings clear -all",
+
             # Disable Spotlight lookup. The testsuite creates
             # different binaries with the same UUID, because they only
             # differ in the debug info, which is not being hashed.
             "settings set symbols.enable-external-lookup false",
 
             # Disable fix-its by default so that incorrect expressions in tests don't
             # pass just because Clang thinks it has a fix-it.
             "settings set target.auto-apply-fixits false",

lldb/source/Commands/CommandObjectSettings.cpp

if (error.Fail()) {
       return false;
     }
 
     return result.Succeeded();
   }
 };
 
 // CommandObjectSettingsClear
+#define LLDB_OPTIONS_settings_clear
+#include "CommandOptions.inc"
 
 class CommandObjectSettingsClear : public CommandObjectParsed {
 public:
   CommandObjectSettingsClear(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "settings clear",
-            "Clear a debugger setting array, dictionary, or string.", nullptr) {
+            "Clear a debugger setting array, dictionary, or string. "
+            "If '-a' option is specified, it clears all settings.", nullptr) {
     CommandArgumentEntry arg;
     CommandArgumentData var_name_arg;
 
     // Define the first (and only) variant of this arg.
     var_name_arg.arg_type = eArgTypeSettingVariableName;
     var_name_arg.arg_repetition = eArgRepeatPlain;
 
     // There is only one variant this argument could be; put it into the
HandleArgumentCompletion(CompletionRequest &request,
                            OptionElementVector &opt_element_vector) override {
     // Attempting to complete variable name
     if (request.GetCursorIndex() < 2)
       CommandCompletions::InvokeCommonCompletionCallbacks(
           GetCommandInterpreter(), CommandCompletions::eSettingsNameCompletion,
           request, nullptr);
   }
 
+   Options *GetOptions() override { return &m_options; }
+
+  class CommandOptions : public Options {
+  public:
+    CommandOptions() = default;
+
+    ~CommandOptions() override = default;
+
+    Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+                          ExecutionContext *execution_context) override {
+      const int short_option = m_getopt_table[option_idx].val;
+      switch (short_option) {
+      case 'a':
+        m_clear_all = true;
+        break;
+      default:
+        llvm_unreachable("Unimplemented option");
+      }
+      return Status();
+    }
+
+    void OptionParsingStarting(ExecutionContext *execution_context) override {
+      m_clear_all = false;
+    }
+
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+      return llvm::makeArrayRef(g_settings_clear_options);
+    }
+
+    bool m_clear_all = false;
+  };
+
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
     const size_t argc = command.GetArgumentCount();
 
+    if (m_options.m_clear_all) {
+      if (argc != 0) {
+        result.AppendError("'settings clear --all' doesn't take any arguments");
+        result.SetStatus(eReturnStatusFailed);
+        return false;
+      }
+      GetDebugger().GetValueProperties()->Clear();
+      return result.Succeeded();
+    }
+
     if (argc != 1) {
       result.AppendError("'settings clear' takes exactly one argument");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     const char *var_name = command.GetArgumentAtIndex(0);
     if ((var_name == nullptr) || (var_name[0] == '\0')) {
       result.AppendError("'settings clear' command requires a valid variable "
                          "name; No value supplied");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     Status error(GetDebugger().SetPropertyValue(
         &m_exe_ctx, eVarSetOperationClear, var_name, llvm::StringRef()));
     if (error.Fail()) {
       result.AppendError(error.AsCString());
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     return result.Succeeded();
   }
+
+  private:
+    CommandOptions m_options;
 };
 
 // CommandObjectMultiwordSettings
 
 CommandObjectMultiwordSettings::CommandObjectMultiwordSettings(
     CommandInterpreter &interpreter)
     : CommandObjectMultiword(interpreter, "settings",
                              "Commands for managing LLDB settings.",

lldb/source/Commands/Options.td

 }
 
 let Command = "settings read" in {
   def setread_file : Option<"file", "f">, Required, Arg<"Filename">,
     Completion<"DiskFile">,
     Desc<"The file from which to read the settings.">;
 }
 
+let Command = "settings clear" in {
+  def setclear_all : Option<"all", "a">,
+    Desc<"Clear all settings.">;
+}
+
 let Command = "breakpoint list" in {
   // FIXME: We need to add an "internal" command, and then add this sort of
   // thing to it. But I need to see it for now, and don't want to wait.
   def blist_internal : Option<"internal", "i">,
     Desc<"Show debugger internal breakpoints">;
   def blist_brief : Option<"brief", "b">, Group<1>,
     Desc<"Give a brief description of the breakpoint (no location info).">;
   def blist_full : Option<"full", "f">, Group<2>,

lldb/test/API/commands/settings/TestSettings.py

class SettingsCommandTestCase(TestBase):
     def test_settings_remove_missing_arg(self):
         self.expect("settings remove", error=True,
                     substrs=["'settings remove' takes an array or dictionary item, or"])
 
     def test_settings_remove_empty_arg(self):
         self.expect("settings remove ''", error=True,
                     substrs=["'settings remove' command requires a valid variable name"])
 
+    def test_settings_clear_all(self):
+        # Change a dictionary.
+        self.runCmd("settings set target.env-vars a=1 b=2 c=3")
+        # Change an array.
+        self.runCmd("settings set target.run-args a1 b2 c3")
+        # Change a single boolean value.
+        self.runCmd("settings set auto-confirm true")
+        # Change a single integer value.
+        self.runCmd("settings set tab-size 2")
+
+        # Clear everything.
+        self.runCmd("settings clear --all")
+
+        # Check that settings have their default values after clearing.
+        self.expect("settings show target.env-vars", patterns=['^target.env-vars \(dictionary of strings\) =\s*$'])
+        self.expect("settings show target.run-args", patterns=['^target.run-args \(arguments\) =\s*$'])
+        self.expect("settings show auto-confirm", substrs=["false"])
+        self.expect("settings show tab-size", substrs=["4"])
+
+        # Check that the command fails if we combine '--all' option with any arguments.
+        self.expect(
+            "settings clear --all auto-confirm",
+            COMMAND_FAILED_AS_EXPECTED,
+            error=True,
+            substrs=["'settings clear --all' doesn't take any arguments"])
+
     def test_all_settings_exist(self):
         self.expect("settings show",
                     substrs=["auto-confirm",
                              "frame-format",
                              "notify-void",
                              "prompt",
                              "script-lang",
                              "stop-disassembly-count",