This is an archive of the discontinued LLVM Phabricator instance.

Add a NativeProcessProtocol Factory class
ClosedPublic

Authored by labath on Jun 1 2017, 7:21 AM.

Details

Summary

This replaces the static functions used for creating
NativeProcessProtocol instances with a factory pattern, and modernizes
the interface of the new class in the process -- I use llvm::Expected
instead of the Status+value combo. I also move some of the common code
(like the Delegate registration into the base class). The new
arrangement has multiple benefits:

  • it removes the NativeProcess*** dependency from Process/gdb-remote (which for example means that liblldb no longer pulls in this code).
  • it enables unit testing of the GDBRemoteCommunicationServerLLGS class (by providing a mock Native Process).
  • serves as another example on how to use the llvm::Expected class (I couldn't get rid of the Initialize-type functions completely here because of the use of shared_from_this, but that's the next thing on my list here)

Tests still pass on Linux and I've made sure NetBSD compiles after this.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 1 2017, 7:21 AM
labath updated this revision to Diff 101023.Jun 1 2017, 7:23 AM

forgot to reformat the code

zturner added inline comments.Jun 1 2017, 11:04 AM
source/Plugins/Process/Linux/NativeProcessLinux.h
148 ↗(On Diff #101023)

Before it was only returning 1, now it's returning a vector. Any reason?

152 ↗(On Diff #101023)

Shouldn't this be tid_t?

labath added inline comments.Jun 1 2017, 12:15 PM
source/Plugins/Process/Linux/NativeProcessLinux.h
148 ↗(On Diff #101023)

I've refactored the function a bit. It now returns a list of threads that it has attached to. Previously it stored them in the object itself, but now it can't as I don't construct a process object until I know that the attach has succeeded.

I should probably document the return value though.

152 ↗(On Diff #101023)

I'm using native thread types in this code. I think that makes more sense as this is the thing that actually interfaces with system APIs. linux does not have a separate type for thread IDs.

krytarowski added inline comments.Jun 1 2017, 3:01 PM
source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
112 ↗(On Diff #101023)

I can imagine that in some cases there might be returned errno=0 and eg. WIFSIGNALED()=true. wstatus wouldn't be readable for end-user - I think it's better to check for (wpid != -1) following the same code between {} and add dedicated code for (wpid != pid || !WIFSTOPPED(wstatus)) that the state is unexpected and terminating.

133 ↗(On Diff #101023)

ReinitializeThreads() was designed to not set stopped reason in threads.

  for (const auto &thread_sp : m_threads) {
    static_pointer_cast<NativeThreadNetBSD>(thread_sp)->SetStoppedBySignal(
        SIGSTOP);
}

I think we need the equivalent of this code there.

Similar for:

/* Set process stopped */
SetState(StateType::eStateStopped);
834 ↗(On Diff #101023)

Technically it is possible on NetBSD, with uid=root and in kernel in INSECURE mode. This way root can debug init. In all other cases ptrace(2) returns error for PID 1.

I think it should be allowed, or rather not explicitly prohibited behavior.

tools/lldb-server/lldb-gdbserver.cpp
65 ↗(On Diff #101023)

Included twice?

eugene added inline comments.Jun 1 2017, 4:26 PM
source/Plugins/Process/Linux/NativeProcessLinux.h
148 ↗(On Diff #101023)

This method would certainly benefit from a comment that explains the nature of its return value.

labath planned changes to this revision.Jun 2 2017, 7:25 AM

I am going to come back to this later, I'm going to create one or two more cleanup diffs which this will depend on.

labath updated this revision to Diff 105065.Jul 3 2017, 7:23 AM
labath marked 9 inline comments as done.

Address review comments

source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
112 ↗(On Diff #101023)

After further consideration, I believe that waitpid will always succeed because the process launcher will make sure for us that the process has actually started (so I changed the wpid == -1 part into an assert). The only part that can fail here is the WIFSTOPPED, which can happen if someone kills the child process before we manage to stop it (in which case we will get a WIFEXITED/WIFSIGNALED instead.

133 ↗(On Diff #101023)

That sounds correct. I have the equivalent code in the attach case, I must have missed this one somehow.

834 ↗(On Diff #101023)

Agreed. In fact, I have removed the same check from linux code. If the user has enough privileges, and really wants to debug init, I don't think we should stop him.

krytarowski added inline comments.Jul 3 2017, 8:10 AM
source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
85 ↗(On Diff #105065)

We can set BSD specific: -1 -> WAIT_ANY.

I verified that it exists on NetBSD, FreeBSD and OpenBSD.

87 ↗(On Diff #105065)

Is this line needed?

738 ↗(On Diff #105065)

We can set BSD specific: -1 -> WAIT_ANY.

labath marked 6 inline comments as done.Jul 3 2017, 8:36 AM
labath added inline comments.
source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
85 ↗(On Diff #105065)

I think that's semantically incorrect, WAIT_ANY means "any process" when used in the pid argument of waitpid. The -1 here is the "failure value" returned by waitpid, and that one is documented as -1 even on netbsd.

87 ↗(On Diff #105065)

Yes, otherwise you get unused variable warnings in no-asserts build. (Or at least you would get, if I had removed the wpid != pid check like I was planning to).

labath updated this revision to Diff 105095.Jul 3 2017, 8:55 AM
labath marked 2 inline comments as done.

remove extra pid check

krytarowski added inline comments.Jul 3 2017, 9:00 AM
source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
85 ↗(On Diff #105065)

Right, my mistake.

krytarowski accepted this revision.Jul 3 2017, 9:16 AM
krytarowski added inline comments.
source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
87 ↗(On Diff #105065)

This is right, I viewed a differential commit and it was misleading.

This revision is now accepted and ready to land.Jul 3 2017, 9:16 AM
This revision was automatically updated to reflect the committed changes.