[lldb] Fix that empty target.run-args are not actually used when launching process

[teemperor]

Summary

GetPropertyAtIndexAsArgs returns true on success and false on failure. Right
now it returns the converted size_t returned from GetArgs which describes
the number of arguments in the argument list. So for empty argument lists
((size_t)0 -> (bool)false) this function always fails.

The only observable effect of this seems to be that empty arguments are never
propagated to the internal LaunchInfo for a process. This causes that once any
argument has been added to target.run-args, clearing target.run-args doesn't
have any effect.

Fixes issue #55568

[teemperor]

Don't have access to a macOS machine that can run the tests, so maybe someone should give this a spin before landing :)

lldb/source/Interpreter/OptionValueProperties.cpp

257–260

The fix for the test is the change a few lines above. This change and the one below seem to have the same bug. Not sure if any properties are using this code at the moment, so technically this is an untested chance. I can drop if people care.

[JDevlieghere]

LGTM. Passes the test suite on macOS.

[jingham]

GetPropertyAtIndexAsArg isn't actually documented, so we don't know what the original intent for the return value was. But in almost all the cases where we look up a property using this function, when we use the enum directly we discard the result. In the only other case (GetUserSpecifiedTrapHandlerNames) were we do return the result of the GetProperty... function, the return result of the GetUser... function is always discarded. So it seems pretty clear that the result was just supposed to be "couldn't find the property". It's also weird to conflate "couldn't find property" with "property has an empty value".

But then that leads one to wonder why TargetProperties::GetRunArguments is returning a bool not a void. Does that relay any useful information? It's not like we're going to remove the property at ePropertyRunArgs but leave the GetRunArguments function around, thus rendering useful the check on return value? And if the intent was to return info about elements in the array, returning the number of elements makes some sense, but an "empty" bool is odd and not in line with the way we usually do things.

The other question is - given that this function used to mean "true if there was more than one arg, false if there were none" - whether there was any reason why when someone reset the target run-args to an empty args set we wouldn't want to update the launch info. That really seemed the intent of the original code. Was there some instance where we would inadvertently empty the run args property w/o wanting to update the next run of that target? Anyway, I couldn't think of any good reason for that, and it clearly has this undesirable side effect. So I think that was just a case of "it has a return value I should check it" rather than intending to do something special for empty run args.

TL;DR This looks okay to me. I think it would be good to also change GetRunArguments to remove the now useless bool return value, but the patch is also fine without that. I think you should keep the other changes, it would be confusing for this to be inconsistent and it's not like those changes are going to have subtle bugs in the change.

I'd keep the other two changes. We should be consistent, and it's not like this change itself is likely to have subtle bugs.

lldb/source/Interpreter/OptionValueProperties.cpp

bool OptionValueProperties::GetPropertyAtIndexAsArgs(
   if (!property)
     return false;
 
   OptionValue *value = property->GetValue().get();
   if (!value)
     return false;
 
   const OptionValueArgs *arguments = value->GetAsArgs();
-  if (arguments)
-    return arguments->GetArgs(args);
+  if (arguments) {
+    arguments->GetArgs(args);
+    return true;
+  }
 
   const OptionValueArray *array = value->GetAsArray();
-  if (array)
-    return array->GetArgs(args);
+  if (array) {
+    array->GetArgs(args);
+    return true;
+  }
 
   const OptionValueDictionary *dict = value->GetAsDictionary();
-  if (dict)
-    return dict->GetArgs(args);
+  if (dict) {
+    dict->GetArgs(args);
+    return true;
+  }
 
   return false;
 }
 
 bool OptionValueProperties::SetPropertyAtIndexFromArgs(
     const ExecutionContext *exe_ctx, uint32_t idx, const Args &args) {
   const Property *property = GetPropertyAtIndex(exe_ctx, true, idx);
   if (!property)

lldb/test/API/python_api/target/TestTargetAPI.py

def test_launch_simple(self):
         process = target.LaunchSimple(None, None,
                                       self.get_process_working_directory())
         process.Continue()
         self.assertState(process.GetState(), lldb.eStateExited)
         output = process.GetSTDOUT(9999)
         self.assertIn('arg: foo', output)
         self.assertIn('env: bar=baz', output)
 
+        # Clear all the run args set above.
+        self.runCmd("setting clear target.run-args")
+        process = target.LaunchSimple(None, None,
+                                      self.get_process_working_directory())
+        process.Continue()
+        self.assertEqual(process.GetState(), lldb.eStateExited)
+        output = process.GetSTDOUT(9999)
+        self.assertNotIn('arg: foo', output)
+
         self.runCmd("settings set target.disable-stdio true")
         process = target.LaunchSimple(
             None, None, self.get_process_working_directory())
         process.Continue()
         self.assertState(process.GetState(), lldb.eStateExited)
         output = process.GetSTDOUT(9999)
         self.assertEqual(output, "")