This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][GUI] Add Process Launch form
ClosedPublic

Authored by OmarEmaraDev on Aug 10 2021, 2:31 PM.

Details

Summary

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.

Diff Detail

Event Timeline

OmarEmaraDev created this revision.Aug 10 2021, 2:31 PM
OmarEmaraDev requested review of this revision.Aug 10 2021, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 2:31 PM

This is not fully working yet, and I am still debugging it. But I thought I would push it for early feedback regardless.

clayborg added inline comments.Aug 10 2021, 2:58 PM
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()

OmarEmaraDev added inline comments.Aug 11 2021, 1:14 AM
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.

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.

  • Address review.

@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.

@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.

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"

clayborg added inline comments.Aug 11 2021, 3:05 PM
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(...)

@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.

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"

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.

OmarEmaraDev added inline comments.Aug 12 2021, 1:17 AM
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?

  • Separate environment field into two fields.
clayborg requested changes to this revision.Aug 17 2021, 9:30 PM

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.

This revision now requires changes to proceed.Aug 17 2021, 9:30 PM
  • Address review
clayborg accepted this revision.Aug 18 2021, 3:07 PM

Work well now on macOS.

This revision is now accepted and ready to land.Aug 18 2021, 3:07 PM
This revision was landed with ongoing or failed builds.Aug 18 2021, 3:43 PM
This revision was automatically updated to reflect the committed changes.