This is an archive of the discontinued LLVM Phabricator instance.

[lldb] common completion for process pids and process names
ClosedPublic

Authored by MrHate on May 28 2020, 2:11 AM.

Details

Summary
  1. Added two common completions: ProcessIDs and ProcessNames, which are refactored from their original dedicated option completions;
  2. Removed the dedicated option completion functions of process attach and platform process attach, so that they can use arg-type-bound common completions instead;
  3. Bound eArgTypePid to the pid completion, eArgTypeProcessName to the process name completion in CommandObject.cpp;
  4. Added a related test case.

Diff Detail

Event Timeline

MrHate created this revision.May 28 2020, 2:11 AM
MrHate updated this revision to Diff 268760.Jun 5 2020, 5:54 AM

Wrap pid completion as a common completion.

JDevlieghere added inline comments.Jun 9 2020, 9:53 AM
lldb/test/API/functionalities/completion/TestCompletion.py
101

Why is this necessary?

105

If self.complete_from_to fails and raises and exception, the process potentially won't terminate. Can you wrap this in a try-except-finally? You should be able to just re-raise the exception in the except clause.

MrHate added a comment.Jun 9 2020, 6:12 PM
  1. For the necessity it's because lldb does filter out its own pid and the pid of the process it already attached to, so I create a process here to provide an available pid which is known to us and able to be found by the completion implementation.
  2. For the child process termination problem, with the process flag value daemon set to True, this child process should be terminated automatically if this test process is killed due to any exception.

If I did misunderstand your meaning or my solution is not considerate, please be in more detail, thx!

teemperor requested changes to this revision.Jul 1 2020, 7:59 AM

So just to give some additional explanation for the Process thing: LLDB doesn't provide the pid for itself when listing process to attach to (which would just break LLDB if the user actually attaches) and there isn't really any other pid we know to exist on the target system (we could speculate 1 is always running, but that seems a bit sketchy). And a program which we already stopped is apparently also not in the list (we can't double attach). So this test has to launch some process that we know is waiting for us and that hasn't an attached LLDB. @MrHate I think this should be documented in the test.

Beside that this seems alright to me beside some minor nits.

lldb/source/Commands/CommandCompletions.cpp
627

You can do a C++11 for loop over the process_infos (it's just a std::vector).

lldb/test/API/functionalities/completion/TestCompletion.py
99

Could this just be some kind of sleep or something else that doesn't actively use CPU?

This revision now requires changes to proceed.Jul 1 2020, 7:59 AM
MrHate updated this revision to Diff 275479.Jul 3 2020, 9:14 PM
  1. modified for loop;
  2. replaced pass with time.sleep(1) so that the test process won't take up CPU.
teemperor requested changes to this revision.Jul 6 2020, 2:37 AM

LGTM, but I have some minor things. I would do them for you when merging but I just want to make sure you see them so I'll send this review back to you :)

lldb/test/API/functionalities/completion/TestCompletion.py
96

This test probably won't work in remote test suite runs (which is essentially when LLDB's test suit is run but all the target programs are on another machine. Usually that's used for testing debugging on a phone). Mainly because the process we create here will be created on the system where the test runs and not the remote machine. But I also don't think there is a nice way to create a dummy process that we can attach to on a remote machine, so I think this is fine. But you should add a @skipIfRemote to the test that we don't see the test failures when someone runs the tests on a remote machine.

So like this:

@skipIfRemote
def test_process_attach_dash_p(self):

(You probably already saw the @skipIf... markers in LLDB tests. Those are just used to disable the test on certain configurations where they are not supposed to pass).

101

Can you add a comment that this is done because we need to have a process that we can always attach to (and that LLDB itself and an already attached process is out of question because we can't attach to those).

This revision now requires changes to proceed.Jul 6 2020, 2:37 AM
MrHate updated this revision to Diff 275879.Jul 6 2020, 6:29 PM
  1. added @skipIfRemote to the test case;
  2. added the comment explaining the process creation.
MrHate updated this revision to Diff 276392.Jul 8 2020, 5:50 AM

clang-formatted.

MrHate updated this revision to Diff 278968.EditedJul 17 2020, 9:53 PM
  • Set daemon to True on creating process, so that it will be killed with the test case ending (which was set to False by mistake).
MrHate updated this revision to Diff 279270.Jul 20 2020, 9:04 AM
  • Allow processes with empty names appear in the completion list.
MrHate updated this revision to Diff 279411.Jul 20 2020, 9:25 PM
MrHate edited the summary of this revision. (Show Details)
  • Refactored the whole patch to provide two common completions: pid and process name.
MrHate retitled this revision from [lldb] complete `process attach -p` with PIDs to [lldb] common completion for process pids and process names.Jul 20 2020, 9:32 PM

LGTM now

lldb/test/API/functionalities/completion/TestCompletion.py
105

Why daemon=True should probably be documented. I can do that when merging.

teemperor accepted this revision.Aug 20 2020, 1:20 PM
This revision is now accepted and ready to land.Aug 20 2020, 1:20 PM
MrHate updated this revision to Diff 287153.Aug 21 2020, 8:27 PM
  1. Rebased the patch with the latest master branch from Github;
  2. In test, uses the pid got from psutil.pids()[0] instead of the one generated by creating a new process in case the pickle error.
MrHate updated this revision to Diff 287258.Aug 23 2020, 7:45 AM
  • Refactored the test with spawnSubprocess and modified main.cpp.
This revision was automatically updated to reflect the committed changes.