Handle trailing spaces on "settings set" command more correctly

[labath]

Summary

Currently we have some settings which treat "\ " on settings set commands specially. E.g., it is
a valid way of specifying an argument of " " to a target. However, this fails if "\ " is the last
argument as CommandObjectSettingsSet strips trailing whitespace. This resulted in a surprising
argument of "\" to the target.

This patch disables the training whitespace removal at a global
level. Instead, for each argument type we locally determine whether whitespace stripping makes
sense. Currently, I strip whitespace for all simple object type except of regex and
format-string, with the rationale that these two object types do their own complex parsing and we
want to interfere with them as least as possible. Specifically, stripping the whitespace of a
regex "\ " will result in a (surprising?) error "trailing backslash". Furthermore, the default
value of dissasembly-format setting already contains a trailing space and there is no way for the
user to type this in manually if we strip whitespace.

[clayborg]

See inline code comments.

Overall good patch, we should not switch just one (StringToBoolean) over to use llvm::StringRef unless we plan to change all of them. I would be happy, possibly in another patch to see:

OptionValue::SetValueFromCString(const char *s, ...)

be switched over to:

OptionValue::SetValueFromString(llvm::StringRef str, ...)

include/lldb/Interpreter/Args.h

21 ↗

(On Diff #19835)

Remove this if you agree with my comments on line 385.

385 ↗

(On Diff #19835)

Unless we are going to change all StringTo calls with llvm::StringRef, I would leave this as a "const char *" and just make a local llvm::StringRef inside StringToBoolean.

source/Interpreter/Args.cpp

871

We should use "const char *" for this arg unless we plan to switch all StringTo functions over to use llvm::StringRef and just make a local llvm::StringRef.

874–877

We should use equals_lower(...) instead of compare_lower here.

884–887

We should use equals_lower(...) instead of compare_lower here.

[labath]

Address review comments.

I will follow this up with a SetValueFromCString -> StringRef patch

source/Commands/CommandObjectSettings.cpp

DoExecute (const char *command, CommandReturnObject &result)
             result.AppendError ("'settings set' command requires a valid variable name");
             result.SetStatus (eReturnStatusFailed);
             return false;
         }
 
         // Split the raw command into var_name and value pair.
         llvm::StringRef raw_str(command);
         std::string var_value_string = raw_str.split(var_name).second.str();
-        const char *var_value_cstr = Args::StripSpaces(var_value_string, true, true, false);
+        const char *var_value_cstr = Args::StripSpaces(var_value_string, true, false, false);
 
         Error error;
         if (m_options.m_global)
         {
             error = m_interpreter.GetDebugger().SetPropertyValue (NULL,
                                                                   eVarSetOperationAssign,
                                                                   var_name,
                                                                   var_value_cstr);

source/Interpreter/Args.cpp

if (!s.empty())
         }
     }
     if (return_null_if_empty && s.empty())
         return nullptr;
     return s.c_str();
 }
 
 bool
 Args::StringToBoolean (const char *s, bool fail_value, bool *success_ptr)
 {
-    if (s && s[0])
-    {
-        if (::strcasecmp (s, "false") == 0 ||
-            ::strcasecmp (s, "off") == 0 ||
-            ::strcasecmp (s, "no") == 0 ||
-                ::strcmp (s, "0") == 0)
-        {
-            if (success_ptr)
-                *success_ptr = true;
-            return false;
-        }
-        else
-        if (::strcasecmp (s, "true") == 0 ||
-            ::strcasecmp (s, "on") == 0 ||
-            ::strcasecmp (s, "yes") == 0 ||
-                ::strcmp (s, "1") == 0)
-        {
-            if (success_ptr) *success_ptr = true;
-            return true;
-        }
-    }
+    llvm::StringRef ref = llvm::StringRef(s).trim();
+    if (ref.equals_lower("false") ||
+        ref.equals_lower("off") ||
+        ref.equals_lower("no") ||
+        ref.equals_lower("0"))
+    if (ref.equals_lower("true") ||
+        ref.equals_lower("on") ||
+        ref.equals_lower("yes") ||
+        ref.equals_lower("1"))
     if (success_ptr) *success_ptr = false;
     return fail_value;
 }
 
 char
 Args::StringToChar(const char *s, char fail_value, bool *success_ptr)
 {
     bool success = false;

source/Interpreter/OptionValueArch.cpp

OptionValueArch::SetValueFromCString (const char *value_cstr, VarSetOperationType op)
     {
     case eVarSetOperationClear:
         Clear();
         NotifyValueChanged();
         break;
         
     case eVarSetOperationReplace:
     case eVarSetOperationAssign:
-        if (value_cstr && value_cstr[0])
+        if (value_cstr)
         {
+            std::string value = llvm::StringRef(value_cstr).trim().str();
+            value_cstr = value.c_str();
             if (m_current_value.SetTriple (value_cstr))
             {
                 m_value_was_set = true;
                 NotifyValueChanged();
             }
             else
                 error.SetErrorStringWithFormat("unsupported architecture '%s'", value_cstr);
         }

source/Interpreter/OptionValueEnumeration.cpp

switch (op)
             Clear ();
             NotifyValueChanged();
             break;
             
         case eVarSetOperationReplace:
         case eVarSetOperationAssign:
             if (value && value[0])
             {
-                ConstString const_enumerator_name(value);
+                ConstString const_enumerator_name(llvm::StringRef(value).trim());
                 const EnumerationMapEntry *enumerator_entry = m_enumerations.FindFirstValueForName (const_enumerator_name.GetCString());
                 if (enumerator_entry)
                 {
                     m_current_value = enumerator_entry->value.value;
                     NotifyValueChanged();
                 }
                 else
                 {

source/Interpreter/OptionValueSInt64.cpp

switch (op)
             Clear();
             NotifyValueChanged();
             break;
             
         case eVarSetOperationReplace:
         case eVarSetOperationAssign:
             {
                 bool success = false;
-                int64_t value = StringConvert::ToSInt64 (value_cstr, 0, 0, &success);
+                std::string value_str = llvm::StringRef(value_cstr).trim().str();
+                int64_t value = StringConvert::ToSInt64 (value_str.c_str(), 0, 0, &success);
                 if (success)
                 {
                     if (value >= m_min_value && value <= m_max_value)
                     {
                         m_value_was_set = true;
                         m_current_value = value;
                         NotifyValueChanged();
                     }

source/Interpreter/OptionValueString.cpp

 
 Error
 OptionValueString::SetValueFromCString (const char *value_cstr,
                                         VarSetOperationType op)
 {
     Error error;
 
     std::string value_str_no_quotes;
-    if (value_cstr)
+    llvm::StringRef trimmed = value_cstr ? llvm::StringRef(value_cstr).trim() : llvm::StringRef();
+    if (trimmed.size() > 0)
     {
-        switch (value_cstr[0])
+        switch (trimmed.front())
         {
         case '"':
         case '\'':
             {
-                size_t len = strlen(value_cstr);
-                if (len <= 1 || value_cstr[len-1] != value_cstr[0])
+                if (trimmed.size() <= 1 || trimmed.back() != trimmed.front())
                 {
                     error.SetErrorString("mismatched quotes");
                     return error;
                 }
-                value_str_no_quotes.assign (value_cstr + 1, len - 2);
-                value_cstr = value_str_no_quotes.c_str();
+                trimmed = trimmed.drop_front().drop_back().str();
             }
             break;
         }
+        value_str_no_quotes = trimmed.str();
+        value_cstr = value_str_no_quotes.c_str();
     }
 
     switch (op)
     {
     case eVarSetOperationInvalid:
     case eVarSetOperationInsertBefore:
     case eVarSetOperationInsertAfter:
     case eVarSetOperationRemove:

source/Interpreter/OptionValueUInt64.cpp

switch (op)
             Clear ();
             NotifyValueChanged();
             break;
             
         case eVarSetOperationReplace:
         case eVarSetOperationAssign:
         {
             bool success = false;
-            uint64_t value = StringConvert::ToUInt64 (value_cstr, 0, 0, &success);
+            std::string value_str = llvm::StringRef(value_cstr).trim().str();
+            uint64_t value = StringConvert::ToUInt64 (value_str.c_str(), 0, 0, &success);
             if (success)
             {
                 m_value_was_set = true;
                 m_current_value = value;
                 NotifyValueChanged();
             }
             else
             {

test/settings/TestSettings.py

def test_settings_with_trailing_whitespace (self):
         # arguments
         self.runCmd ("settings set target.run-args 1 2 3")  # Set to known value
         self.runCmd ("settings set target.run-args 3 4 5 ") # Set to new value with trailing whitespaces
         self.expect ("settings show target.run-args", SETTING_MSG("target.run-args"),
             substrs = [ 'target.run-args (arguments) =', 
                         '[0]: "3"', 
                         '[1]: "4"',
                         '[2]: "5"' ])
+        self.runCmd ("settings set target.run-args 1 2 3")  # Set to known value
+        self.runCmd ("settings set target.run-args 3 \  \ ") # Set to new value with trailing whitespaces
+        self.expect ("settings show target.run-args", SETTING_MSG("target.run-args"),
+            substrs = [ 'target.run-args (arguments) =',
+                        '[0]: "3"',
+                        '[1]: " "',
+                        '[2]: " "' ])
         self.runCmd("settings clear target.run-args", check=False)        
         # dictionaries
         self.runCmd ("settings clear target.env-vars")  # Set to known value
         self.runCmd ("settings set target.env-vars A=B C=D\t ") # Set to new value with trailing whitespaces
         self.expect ("settings show target.env-vars", SETTING_MSG("target.env-vars"),
             substrs = [ 'target.env-vars (dictionary of strings) =', 
                         'A=B', 
                         'C=D'])
         self.runCmd("settings clear target.env-vars", check=False)        
+        # regex
+        self.runCmd ("settings clear target.process.thread.step-avoid-regexp")  # Set to known value
+        self.runCmd ("settings set target.process.thread.step-avoid-regexp foo\\ ") # Set to new value with trailing whitespaces
+        self.expect ("settings show target.process.thread.step-avoid-regexp",
+                SETTING_MSG("target.process.thread.step-avoid-regexp"),
+                substrs = [ 'target.process.thread.step-avoid-regexp (regex) = foo\\ '])
+        self.runCmd("settings clear target.process.thread.step-avoid-regexp", check=False)
+        # format-string
+        self.runCmd ("settings clear disassembly-format")  # Set to known value
+        self.runCmd ("settings set disassembly-format foo ") # Set to new value with trailing whitespaces
+        self.expect ("settings show disassembly-format",
+                SETTING_MSG("disassembly-format"),
+                substrs = [ 'disassembly-format (format-string) = "foo "'])
+        self.runCmd("settings clear disassembly-format", check=False)
         
     def test_all_settings_exist (self):
         self.expect ("settings show",
                      substrs = [ "auto-confirm",
                                  "frame-format",
                                  "notify-void",
                                  "prompt",
                                  "script-lang",