This patch adds a process launch form. Additionally, a LazyBoolean field
was implemented and numerous utility methods were added to various
fields to get the launch form working.
Details
- Reviewers
clayborg teemperor - Commits
- rG82507f179876: [LLDB][GUI] Add Process Launch form
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This is not fully working yet, and I am still debugging it. But I thought I would push it for early feedback regardless.
lldb/source/Core/IOHandlerCursesGUI.cpp | ||
---|---|---|
3133 | We should fill in any arguments automatically into this field from the target and let the user modify them as needed. | |
3134 | We should fill in any current environment variables from the target if we aren't already doing this. | |
3138 | Set default values for all fields in here from the target if/when possible. | |
3151 | Maybe label as "Redirect Standard Output" or "Standard Output File"? | |
3154 | If you change above field, then modify this one as well. | |
3157 | If you change above field, then modify this one as well. | |
3168 | I'd pull working directory out of advanced settings. | |
3191 | Should this be labeled "GetArguments"? The name seems like it would set the arguments in this class from launch_info, but it is getting them from this class and filling them into "launch_info". Ditto for all accessors that fill in "launch_info" below. | |
3265–3268 | We don't need to do anything if this isn't specified as by default the input will be hooked up by the debugging to something valid. If the users wants to redirect to /dev/null, they can just specify "/dev/null" (or the right path for this on their system. | |
3274–3277 | Ditto above, don't do anything if this isn't specified. | |
3283–3286 | Ditto above, don't do anything if this isn't specified. | |
3292–3293 | This is an obscure MacOS specific setting, so no need to make a field of it like suggested below. | |
3295–3296 | We should make a Boolean input field in the advanced settings, and the default should be set to "target->GetDetachOnError()". Then the user can modify this if needed. | |
3298–3299 | This should be moved above all of the STDIO redirection stuff above since it will cause everything to be redirected to /dev/null or equivalent. We should make a boolean setting for this in the advanced stuff, and if this is set to true, then don't show the output redirection fields (m_standard_input_field, m_standard_output_field and m_standard_error_field) in UpdateFieldsVisibility() |
lldb/source/Core/IOHandlerCursesGUI.cpp | ||
---|---|---|
3265–3268 | But since we are in GUI mode, what will the standard files be hooked into? It seems the only two options is to either redirect to a file or to /dev/null, hence the condition I have. Is this not the case? |
One thing to note as well is that the target environment seem to include many environment variables like PATH and HOME, I don't think those should be included.
If I am debugging a program on my local machine then I do want the program to start up with reasonable values for PATH and HOME, DISPLAY and suchlike or it won't run correctly. Since people don't tend to have a lot of "debugger specific environment" variables set, it's just a fairly vanilla command-line tool in that regard, the debugger's environment is a fine thing to use to prime the inferior. Note, if there are certain environment variables that you don't want to pass from the debugger to the inferior, use the setting target.unset-env-vars.
Of course, if you are debugging remotely to a device or to another machine, you probably don't want to share many (or any) env-vars with the inferior. In that use-case, you set target.inherit-env to 0 and fill in all the necessary ones with target.env-vars.
But I think the most common use-case is local debugging, and lldb is usually run with a fairly reasonable environment, so having target.inherit-env be true by default is the right choice.
@jingham I wasn't arguing that we should remove those environment variables, on the contrary. Greg was suggesting that we populate the environment field with the target environment instead of implicitly adding them to the environment of the launch info. The problem with that is that there is a large number of environments that gets added (15 in my case) as shown in the following screenshot. What I was saying in my comment above is that I don't think we should show those to the user, we should transparently add them instead and let the user add additional variables as needed. We can then look into adding other settings to exclude them if necessary.
Thanks for the clarification.
I still think people will want to see the actual environment that is being provided to the inferior. They don't want to have to do our merging algorithm in their heads.
For instance, if lldb had an environment variable that wasn't appropriate for the inferior, I would want to see that in the Environment View, so I could turn it off. If that variable is passed silently to the inferior, and not shown anywhere in the launch UI, figuring out that I needed to not inherit that one would be more difficult.
However, if you feel like the really useful affordance here is for "User Provided Environment Variables", that's also a valid choice, but then you should name it appropriately, so as not to cause confusion. If there's a window listing in the launch parameters panel called "Environment Variables" it should be the environment variables the process will launch with, since that's the plain meaning of the title.
I was actually suggesting only priming the env var list with what was contained in the "target.env-vars" setting. This doesn't include the extra env vars that are inherited. Just the ones that the user explicitly set. That defaults to nothing usually. If we go this way, then we should add a new boolean for inheriting the env vars that defaults to the current value of "target.inherit-env"
lldb/source/Core/IOHandlerCursesGUI.cpp | ||
---|---|---|
3221 | Does this currently get all target env vars including the inherited ones? | |
3231 | Use FileSpec::GetPath(), it already returns a std::string. The current code could crash if it returns NULL | |
3268 | They get delivered to the Process class itself. We can later make a window for process STDIO if we need to. But the STDIO will live inside the lldb_private::Process class and get stored. It can be fetched with Process::GetSTDOUT(...) and Process::GetSTDERR(...) |
I think it would be confusing to have a window called "Environment Variables" that has a checkbox that says "Inherit Environment" and when I check or uncheck that checkbox the "Environment Variables" pane contents doesn't change. We had this same confusion internally and in command-line lldb for quite a while, but that was causing confusion and we sorted that all out a year or so ago. We shouldn't replicate this confusion in the UI.
There's some consistency in having a pane in the Launch Parameters that allows you to add "User-Specified Environment Variables" as well as a checkbox to inherit the environment. After all, you can't edit the environment variables you inherit other than by overriding them or unsetting them with the unset-env-vars setting. So this would be a consistent set of editable parameters. But it needs to be clear what this is actually displaying.
It would still be nice if the UI gave a way to see the resultant target environment. Maybe a "show inherited env-vars" control to go along with the "inherit-env" checkbox? But it's not terrible if I have to go out to the console to see that.
lldb/source/Core/IOHandlerCursesGUI.cpp | ||
---|---|---|
3221 | Yes. It seems the logic in TargetProperties::ComputeEnvironment adds the inherited environment, erase the unset environment, and finally add the target environment. If we want the target environment only, we can add another method to TargetProperties to get only those, or we could erase the platform environment elements from the computed environment. |
Adding a boolean for inheriting the environment may not be necessary, we can just add two environment fields, one for inherited and one for user specified. The inherited one will be put in advanced settings with another boolean that show or hide the field. Both will be filled with the appropriate default values. What do you think?
So I am trying this out. I tried the target creation form and I am not able to use arrow keys when entering the path to the main executable. Are you able to do this?
It seems like all text fields have this issue for me:
- left and right arrow keys don't work
- forward and backward delete don't work
- none of the editing like CTRL+A to go to the beginning work, CTRL+E to go to the end, etc.
Next I tried to launch the process with no args and got a error that was due to use filling in the shell if the user didn't specify it. We need to not specify a shell in the launch information if the user didn't specify it because this will cause us to try and launch the program inside of a shell. See inlined comments for fix.
There is also a big usability issue with this dialog if you check the show advanced settings. You must TAB many many times to get to the "Launch" button.
That being said, lets just fix this dialog for now with by removing the not getting the shell if it isn't specified and not setting the arch in the launch info it it wasn't specified, and this should be functional. I will suggest some usability fixes via email and we can work on getting those resolved with individual patches.
lldb/source/Core/IOHandlerCursesGUI.cpp | ||
---|---|---|
3354–3355 | Don't set the architecture if the user didn't specify it. | |
3361–3362 | We shouldn't fill this out by default. If you do, then all of your programs will be launched via a shell. On Mac, this doesn't work because we can't debug any Apple code signed binairies and "/bin/sh", "/bin/bash", "/bin/zsh" and all others are not allowed to be debugged. So just remove this else clause. After I did this I was able to debug my binary with this dialog. |
We should fill in any arguments automatically into this field from the target and let the user modify them as needed.