This patch adds -exec-arguments command for lldb-mi. -exec-arguments command allows to specify arguments for executable file in MI mode. Also it contains tests for that command.
Btw, new added files was formatted by clang-format.
Differential D6965
Add -exec-arguments command ki.stfu on Jan 14 2015, 6:01 AM. Authored by
Details This patch adds -exec-arguments command for lldb-mi. -exec-arguments command allows to specify arguments for executable file in MI mode. Also it contains tests for that command. Btw, new added files was formatted by clang-format.
Diff Detail
Event TimelineComment Actions Looks ok to me. 2 comments though.
If nobody else comments then I can commit this patch in a day or 2 with modification suggested above. Comment Actions So the way you set arguments when you launch a process through the SB API's (besides some old unstructured API's that we'll eventually deprecate) is through the SBLaunchInfo. You're adding another way to get and set the arguments that's independent of that. I'd rather we didn't create another way to specify launch arguments. You can already do that by creating an SBLaunchInfo with the appropriate args set, and use that for the launch. The SBTarget already provides a way to get the arguments (SBTarget::GetNumArguments/GetArgumentAtIndex.) If you need to provide a way to get the current or previous launch arguments then we should add an API to copy out the SBLaunchInfo that was used for the launch. That's not currently kept around so you may have to reconstruct it from bits sitting in the target, but that way we keep one structured way to get at all the launch parameters... Comment Actions Oops. It's the old version. I'll update patch right now.
Ok. I think that I can fix this test using -interpreter-exec command. Comment Actions Update patch:
Comment Actions Yes, I had seen these functions but I was working on CLI support in MI mode. My goal was to fix the following cases: -file-exec-and-symbols ~/p/hello -exec-arguments foo bar -interpreter-exec command "r" Second: -file-exec-and-symbols ~/p/hello -interpreter-exec command "settings set target.run-args foo bar" -exec-run Your way didn't work because when we specify arguments via "settings set target.run-args" they are stored as Debugger's property. Initially I had tried to set and get argument directly but it was awful because SBDebugger::GetInternalVariableValue returns arguments in dumped format, like: [0]: "--arg1" [1]: "2nd arg" [2]: "third_arg" [4]: "fourth=\"4th arg\"" Therefore I decided to extend public API. Comment Actions If we want to have a way to directly set the arguments in a Target, I would prefer to have the SBTarget vend the SBLaunchInfo that it will use when it launches (if you don't give it another SBLaunchInfo) and then set the arguments there. Right now it is pretty incoherent, you can set arguments in the run-args, you can set them directly when you call launch, you can set them in a SBLaunchInfo which you pass to the target, and now you can set then by passing in an SBArgs. That's too many ways to do the same thing. Of the ones that it would be good to reuse, I think the SBLaunchInfo is the best structured, and encapsulates all the various options. So we should expose that rather than make another entity. Comment Actions The "process launch" command uses lldb_private (i.e. not public lldb API) and it creates LaunchInfo every time it is executed. I can't affect to that LaunchInfo instance using the public API.
Maybe it's so but I don't want to create one more way of setting arguments in lldb_private::Target class. I just have made a wrapper for the existing method. Comment Actions My suggestion was to have the SBTarget vend the SBLaunchInfo it WOULD use if you were to call launch without providing an SBLaunchInfo of your own. Unfortunately, the underlying target doesn't actually use a ProcessLaunch info to store these settings, it stores the individual bits. That's purely historical: the ProcessLaunchInfo was a later invention, and was layered onto of the existing code without re-working it. We should probably change that at some point. So if you want extra credit it would be great to re-plumb this stuff. But I can understand if that's way more than you thought you were biting off for this task. I'm still unhappy about adding yet another way to set run arguments, and once it's in the SB API's we're stuck with it for a while. But given that it's our fault we are in this situation, it doesn't seem fair to push this too hard. Jim Comment Actions I would vote for a different approach altogether which would be a cleaner all around implementation: Have the lldb-mi have a variable whose type is: lldb::SBLaunchInfo. lldb::SBLaunchInfo m_launch_info; And somehow you make a function that all command can access this launch info with like: lldb::SBLaunchInfo &GetGlobalLaunchInfo(); This probably should be an instance variable that is part of the global MI interpreter. Any calls to things like: -file-exec-and-symbols ~/p/hello Would do: bool add_as_first_arg = true; GetGlobalLaunchInfo().SetExecutableFile(lldb::SBFileSpec("~/p/hello"), add_as_first_arg); Then: -exec-arguments foo bar would do: const bool append_args = true; GetGlobalLaunchInfo().SetArguments(argv, append_args); And then: -interpreter-exec command "r" would do: lldb::SBProcess process = target.Launch (GetGlobalLaunchInfo(), error); This give you the most flexibility to set/change stuff before you launch, and setting up SBLaunchInfo is the current best practices way to launch a process. Comment Actions
The "r" command is the CLI command which is located in source/Commands/CommandObjectProcess.cpp file. The MI module executes it using SBInterpreter::HandleCommand function and it seems like a user has typed the "r" command in the shell. I think that it's inadmissible to reference to MI module's variable in LLDB's commands because MI it's not a part of LLDB kernel. It just a extra module and LLDB shouldn't depend on it. Comment Actions I stand by my recommendation. There are many commands that build up what is effectively a SBLaunchInfo and eventually a command that makes the program run. It is quite sad to see things like '-interpreter-exec command "r"' in a MI command log. Do you really expect all implementations of MI to implement GDB commands? Is a packet that says "-exe-launch"? The only thing that should be exposing command line execution is when the user types something. And the command interpreter could then be anything and GDB commands would not be expected to work. Greg Comment Actions But your recommendation can't be applied in this case (perhaps I explained it poorly).
I agree that LaunchInfo is much better than many-many arguments of Launch method or than a lot of setters/getters for properties. But current implementation of process commands doesn't allow me to use single LaunchInfo for them and MI. I don't create an additional method for setting of arguments. I just have made a wrapper for the existing method.
huh. Perhaps you didn't face with that but it's impossible to specify an architecture (what can be done by using "target create") or select a remote platform and connect to it (using platform commands) or launch a program and stop at entry (process launch -s) or load a python scripts (using script commands) or ... and so on. Therefore I think '-interpreter-exec command "r"' is an important case which should work too, also as the case when arguments are set up by "settings set target.run-args" after that -exec-run is called. Comment Actions FYI: I looked how run-args are specified for Target class. At the moment it can be done by using "settings set target.run-args" command which sets Debugger's "target.run-args" property. But actually this property is mapped to Target's ePropertyRunArgs property which can be read by Target::GetRunArguments and modified by Target::SetRunArguments. You can see in Target.cpp that Target::Launch gets these arguments by using Target::GetRunArguments. My implementation of "-exec-arguments" uses Target::SetRunArguments (which is wrapped in SBTarget::SetRunArguments) and as I said above it's the same thing. Thus the my implementation works the same way as "settings set target.run-args" and if you don't like it, then this architecture can be rewritten, but it's outside the scope of this CL. Comment Actions The fix I requested should be easy to implement as noted above in the code comments. Step 1: This will provide the cleanest implementation and allow of the most flexibility in setting the environment variables (just modify SBLaunchInfo), working directory (modify SBLaunchInfo), etc, then when you launch you get to just use the launch info you have built up.
Comment Actions I forgot one step: You will need to do a: m_lldbLaunchInfo.SetListener(rSessionInfo.m_rLlldbListener); before you call lldb::SBProcess process = m_lldbTarget.Launch(m_launch, error); Comment Actions
Greg, we are talking about different things. Your patch looks good but it wouldn't work on test case which I described earlier: Comment Actions Greg, I know that it takes a long your time but we didn't decide what need to do with this patch. If you tell me that "process launch" shouldn't work as I have described above then I can remove SBTarget::SetRunArguments, but I think it should. Sorry that I didn't provide complete information in the description at once. Comment Actions So remove everything I listed and add m_lldbLaunchOptions to your CMICmnLLDBDebugSessionInfo object and populate it with all of the -exec calls, then use it to launch
Comment Actions I have updated the patch according to your remarks. Hope it looks good for you. But I have a problem: In Diff 4 all tests pass, but now the MiInterpreterExecTestCase.test_lldbmi_settings_set_target_run_args test fails. What we should do with it? FYI: I was trying to explain this problem in my comments above. Comment Actions When you launch the SBTarget run args will be set for you from your SBLaunchInfo, so we don't really need to test that the setting gets set explicitly as "settings set target.run-args" is. What were you trying to test with this test? You should be able to get/set the run args in the SBLaunchInfo now before you run. Is this what you should be testing now? Comment Actions This test checks that we can specify arguments using CLI "settings set target.run-args" command in lldb-mi.
No. I can't get arguments which were set by "settings set target.run-args" command because lldb API doesn't provide this ability. Comment Actions You could verify that you can set the args with "settings set" and then run with your empty SBLaunchInfo and verify that you got the right args? Comment Actions It would be nice to have lldb_private::Target have its own ProcessLaunchInfo as a member variable and change all storage of settings (target.run-args, target.arg0 etc) and have the settings just modify the lldb_private::Target's version of this. Then we can add functions to SBTarget: // Get a _copy_ of the target's launch info // Set the target's launch info // Launch using the target's current launch info This would be fine to add. Then you can make your test work again. Comment Actions It doesn't work (test MiInterpreterExecTestCase.test_lldbmi_settings_set_target_run_args by yourself). When I set arguments using "settings set" it were stored as the Debugger's property. And when I call Target::Launch with empty ProcessLaunchInfo, these arguments are ignored. Comment Actions I'll try do it but how I can synchronize Target::m_GlobalLaunchInfo and Debugger's property (which can be set by "settings set" command)? I can't update m_GlobalLaunchInfo every time when "settings set target.run-args" is executed. Therefore will be 2 different places where these arguments are stored. As I said, currently "settings set" stores all values as debugger's properties. And I don't want to do the following: if (property == "target.run-args") { Target::GetGlobalLaunchInfo().SetRunArguments(value); } else { Debugger.SetProperty(...); } Comment Actions Add launch_info to TargetProperties List of changes:
All tests passed on OS X. Comment Actions If we are adding a ProcessLaunchInfo to the TargetProperties everything needs to be kept up to date: { "arg0" , OptionValue::eTypeString , false, 0 , NULL, NULL, "The first argument passed to the program in the argument array which can be different from the executable itself." }, { "run-args" , OptionValue::eTypeArgs , false, 0 , NULL, NULL, "A list containing all the arguments to be passed to the executable when it is run. Note that this does NOT include the argv[0] which is in target.arg0." }, { "env-vars" , OptionValue::eTypeDictionary, false, OptionValue::eTypeString , NULL, NULL, "A list of all the environment variables to be passed to the executable's environment, and their values." }, { "inherit-env" , OptionValue::eTypeBoolean , false, true , NULL, NULL, "Inherit the environment from the process that is running LLDB." }, { "input-path" , OptionValue::eTypeFileSpec , false, 0 , NULL, NULL, "The file/path to be used by the executable program for reading its standard input." }, { "output-path" , OptionValue::eTypeFileSpec , false, 0 , NULL, NULL, "The file/path to be used by the executable program for writing its standard output." }, { "error-path" , OptionValue::eTypeFileSpec , false, 0 , NULL, NULL, "The file/path to be used by the executable program for writing its standard error." }, { "detach-on-error" , OptionValue::eTypeBoolean , false, true , NULL, NULL, "debugserver will detach (rather than killing) a process if it loses connection with lldb." }, { "disable-aslr" , OptionValue::eTypeBoolean , false, true , NULL, NULL, "Disable Address Space Layout Randomization (ASLR)" }, { "disable-stdio" , OptionValue::eTypeBoolean , false, false , NULL, NULL, "Disable stdin/stdout for process (e.g. for a GUI application)" }, You will need to install callback functions for each of these and keep your built in ProcessLaunchInfo up to date.
Comment Actions Added update other properties in m_launch_info
Note, that "inherit-env" doesn't exist in ProcessLaunchInfo. I skipped it.
Comment Actions You will need to get the object for each item individual and set the callback on each OptionValue separately. Right now only only the item OptionValue itself will call the callback it won't pass it up to the parent and call the parent's callback. You can change this to happen if you want. then you just need to install one callback and use the "OptionValue *option_value" second param and look at its name using: ConstString option_name = option_value->GetName(); Then you will need to find out which option it maps to and then set the right value. Comment Actions Inlined comments
Comment Actions Fix callback; Add test that checks "settings set" before "target create"; Fix "process launch -s"; Fix OptionValueArray::DeepCopy |