This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][GUI] Add Process Attach form
ClosedPublic

Authored by OmarEmaraDev on Jul 8 2021, 1:31 PM.

Details

Summary

This patch adds a form window to attach a process, either by PID or by
name. This patch also adds support for dynamic field visibility such
that the form delegate can hide or show certain fields based on some
conditions.

Diff Detail

Event Timeline

OmarEmaraDev created this revision.Jul 8 2021, 1:31 PM
OmarEmaraDev requested review of this revision.Jul 8 2021, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 1:32 PM

I initially created two forms for attach by name and attach by PID, because the options were divided between them. Today I tried to reimplement that such that it is a single form with a choices field at the top that determines if it is by name or PID. The fields are then shown or hidden based on that enum. Additionally, other fields can control the visibility of other fields, for instance, the "Include existing processes" field. I think it works fine and the UX looks good. Here is an example, what do you think?

clayborg requested changes to this revision.Jul 12 2021, 10:39 AM
clayborg added inline comments.
lldb/source/Core/IOHandlerCursesGUI.cpp
1917

Do these actually need to be shared pointers? They can probably become std::unique_ptr<T> objects and the FieldDelegateSP would then become FieldDelegateUP after defining it as a unique_ptr

2185–2200

Spelling issue I missed before

2303

I am guessing that the first item in the list is always selected? Might be nice to provide a way for AddChoicesField to select a different item as the default selection with another optional parameter that is the index of the item that is to be selected. Doesn't have to happen in this diff, just thought of it and wanted to mention this.

2304

remove space

2305

I would be nice if we can eventually auto complete the PID by using the platform to get a list of processes. Sometimes we are not attached to a remote platform, but we are always attached to the host platform and should be able to get a list of process IDs. A new form could be popped up with all of the process details and allowing the user to filter processes with a TextField, then a ChoicesField that contains all valid processes the user could attach to with the pid, process name and possibly other details in the text for each choice. This doesn't need to be done now, but it would be a good extra functionality step to complete later if you get everything done on your list.

2306

remove space

2307

If we have a target, it might be nice to auto populate this field with the basename of the target's main executable.

2308

remove space

2309

Might be nice to iterate over all process plug-ins and make this into a ChoicesField where the first entry is "<default>" and the rest are actual plug-in names. The names can easily be retrieved using:

static const char *GetProcessPluginNameAtIndex(uint32_t idx);

from PluginManager.h

2310

remove space

2349

We might want to pop up a dialog here asking the user if they want to kill or detach since we have a GUI. In the "process attach" command we confirm with the user if the terminal is interactive.
A few ways we can do this:

  • popup a form asking the user to kill/detach/cancel when the user hits submit on this form
  • pop up the dialog before even showing the this form and asking the user to kill or detach, then show this form after that has been handled
  • not allow the Process->Attach to happen by disabling the menu item when a process is running.
2370

We should select the target in he debugger here so that all GUI commands work correctly after this call if they ask the debugger for its selected target.

m_debugger.GetTargetList().SetSelectedTarget(...);
2423–2424

You shouldn't need to do this since you already called ProcessAttachInfo::SetContinueOnceAttached(true) on the attach_info. LLDB should take care of this for you already I think.

This revision now requires changes to proceed.Jul 12 2021, 10:39 AM
OmarEmaraDev marked 10 inline comments as done.
  • Use unique pointers for field delegates.
  • Fix typos in Select methods.
  • Set the process name to the main executable name by default.
  • Turn plugin name into a choice field.
  • Update debugger target if a new one was created.
  • Remove superfluous continue operation.
OmarEmaraDev planned changes to this revision.Jul 13 2021, 4:09 AM

Currently working on the detach/kill form.

lldb/source/Core/IOHandlerCursesGUI.cpp
2349

I think the first option is the best, because it delays this decision at the very end giving the user as much time as possible to cancel attaching without altering the debugger state.

2423–2424

In the command code, there is this snippet of code at the end, so I though it might be necessary:

// This supports the use-case scenario of immediately continuing the
// process once attached.
if (m_options.attach_info.GetContinueOnceAttached())
  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
  • Add kill/detach form.
  • Fix forms with no fields.
OmarEmaraDev marked an inline comment as done.Jul 13 2021, 8:06 AM

Updated UI:

Kill/Detach form.

clayborg requested changes to this revision.Jul 13 2021, 11:53 AM
clayborg added inline comments.
lldb/source/Core/IOHandlerCursesGUI.cpp
1837

You don't want to return a unique_ptr here, if you do something like:

FieldDelegateUP field_up = form->GetField(1);

You have now given away the one reference to this field to the local variable and it will destruct the item when the local goes out of scope.

Easy solution: just return a pointer, not a unique_ptr. The FormDelegate owns all of its fields, so it is ok to hand out a pointer as others should temporarily use the FieldDelegate. Also we should bounds check "field_index".

1988
2155–2156
2161
2178
2210–2211
2217

No need to use a std::unique_ptr here, and GetField should return a pointer as mentioned above.

2234–2235
2276
2300

When we have interaction, we should allow the user to select kill or detach. The GetShouldDetach is only for when the user doesn't interact. Can we use a ChoicesField for this with the two options? We can even go a step further and add three options: "Detach", "Detach (stopped)", and "Kill". This is easy since lldb_private::Process has this option on detach:

Status Process::Detach(bool keep_stopped);

When you detach with "keep_stopped" set to true the process will not resume. This allows another debugger to be attached, or someone to run another tool on the process in its suspended state.

2326

Does anyone remove the sub window if the user cancels this dialog?

2344–2345

most people when attaching won't ever specify the plug-in names. I wonder if we should include an extra boolean field that enables this? Maybe:

m_show_plugin_field = AddBooleanField("Show advanced settings.", false);

And then show/hide any settings that aren't commonly used like the "Plugin Name" field?

2391–2397

While loop might be easier here?

2420

Is the form for the kill/detach done by the time it reaches this code? If so, what if the user cancels the Kill/Detach dialog? Can we return false here if the user cancels?

This revision now requires changes to proceed.Jul 13 2021, 11:53 AM
OmarEmaraDev marked 10 inline comments as done.
  • Return raw pointers instead of unique ones.
  • Add Show Advance Settings option.
  • Allow detaching and killing at the same time.
  • Allow detaching while keeping process stopped.
  • Handle review comments.

Two actions with an option:

lldb/source/Core/IOHandlerCursesGUI.cpp
1837

I see, I though returning it as a reference wouldn't case such issues.

2300

I think this might be a good place to add two actions with possible parameters, let me know if you think this would be a good idea.

2326

Yes. This is done in WindowDelegateHandleChar of the FormWindowDelegate.

2344–2345

This what I do for the launch and target creation forms. In this case, I didn't think it was worth it because it was just one field. But now that it is a choice field that takes a bit more space, maybe it would be a good idea to do this here as well.

2420

No, the return boolean here essentially denotes that a form for killing/detaching was spawned and that the attach form shouldn't continue executing. Once the user executes an action or cancels, the user will end up in the same attach from, where the user can execute attach again where it will succeed without the kill/detach dialog.

I can't think of a way to make the form block execution of another form until it is done without resorting to complicated trickery.

clayborg requested changes to this revision.Jul 13 2021, 3:47 PM

I like the new detach/kill dialog as it incorporates many of your features that you added to the forms! Very close, just a few minor issues and this will be good to go.

lldb/source/Core/IOHandlerCursesGUI.cpp
1837

You are correct unless the caller of the function doesn't use a reference. All of the call sites you had in here were using references, so there wasn't an issue. But if someone else comes along and actually wrote code like the snipped I posted above, it would transfer ownership and that would be bad.

2050–2053
2420

Ok, thanks for the explanation. It makes sense.

2424

Should be easy to verify if it is needed or not. If it is, we will need to add it back.

This revision now requires changes to proceed.Jul 13 2021, 3:47 PM
OmarEmaraDev marked an inline comment as done.
  • Manually continue if needed.
lldb/source/Core/IOHandlerCursesGUI.cpp
2424

Looks like it is actually needed. I added it it back.

clayborg accepted this revision.Jul 15 2021, 2:32 PM

Looks good!

This revision is now accepted and ready to land.Jul 15 2021, 2:32 PM
This revision was landed with ongoing or failed builds.Jul 15 2021, 3:12 PM
This revision was automatically updated to reflect the committed changes.