Sleep is used to make the system sleep the current thread a little bit
between polling for processes by name. If the sleep isn't there, we will
light up a CPU with really quick polling for the processes by name, so
we should use usleep() which take a sleep amount in microseconds to not
peg the CPU at 100% while waiting.
Are there any objections to using std::this_thread::sleep_for? The headers seem to already be included and other tools inside the project seem to make use of it.
Another "issue" I've encountered is related to Ubuntu's ptrace protection [1]. If ptrace is blocked for non-child processes then you get an error. In lldb you'd see "error: attach failed: lost connection" but in the logs of the lldb-server you'd see "GDBRemoteCommunicationServerLLGS::Handle_vAttachWait failed to attach to process named langserver-swift: Operation not permitted". Of note is the "Operation not permitted".
While I'm still not sure how to check for the presence of the ptrace protection (so that a more detailed error can be provided). I think displaying "operation not permitted" is more indicative of the underlying error than "lost connection". So here's the question: is there a way that I could go about surfacing that error back to main lldb?
[1] SecurityTeam/Roadmap/KernelHardening - Ubuntu Wiki
I'm attaching the first cut of the patch that I think is worth sharing for feedback. There is more work to do with regard to documentation and tests. Any feedback is appreciated.
I am going to do my best and follow the steps listed in the "LLVM Developer Policy" as this continues forward.
Greg, Pavel, and Jim thank you very much for the help you have provided thus far. This would have been insurmountable without the guidance.
0001-First-working-attempt.patch (6.69 KB)
sleep_for is definitely better than usleep because of portability (and
we use it elsewhere already).
Could you attach the patch to phabricator instead
<https://reviews.llvm.org/differential/> ? It's easier to make
comments and iterate there. When you upload the patch be sure to
include full context in it (git diff -U9999 or similar). Or you can
use the arcanist tool which will do it for you.
As for the test, there are two test ways you can do that. One is using
the python tests
(test/testcases/tools/lldb-server/TestGdbRemoteAttach.py would be a
good starting point), other is via googletest
(unittests/tools/lldb-server/LLGSTest.cpp). Neither of the tests is
going to be particularly nice because you will have to juggle multiple
things (speaking with the server and launching the inferior), but it
should be doable using both.
We should already be able to display a better error message, if the
server sends one (the Status overload of the SendErrorResponse
function). If the error string that comes out of that is not good
enough, then we can tweak whoever creates the Status object to fill it
out better as a separate patch.
sleep_for is definitely better than usleep because of portability (and
we use it elsewhere already).
Okay that is what I went with.
Could you attach the patch to phabricator instead
<Login; ? It's easier to make
comments and iterate there. When you upload the patch be sure to
include full context in it (git diff -U9999 or similar). Or you can
use the arcanist tool which will do it for you.
I _think_ I have done this correctly.
https://reviews.llvm.org/D47879
$ svn co http://llvm.org/svn/llvm-project/lldb/trunk lldb-trunk
$ patch 0001-First-working-attempt.patch # this is what I mailed last night
$ arc diff
I was not quite sure what I should write for a title or summary. Based on other differentials on the site, the title usually seems to have something in brackets but it is not clear to me what that should be in my case.
As for the test, there are two test ways you can do that. One is using
the python tests
(test/testcases/tools/lldb-server/TestGdbRemoteAttach.py would be a
good starting point), other is via googletest
(unittests/tools/lldb-server/LLGSTest.cpp). Neither of the tests is
going to be particularly nice because you will have to juggle multiple
things (speaking with the server and launching the inferior), but it
should be doable using both.
I will see what I can glean from those examples. See if I can turn them into something.
We should already be able to display a better error message, if the
server sends one (the Status overload of the SendErrorResponse
function). If the error string that comes out of that is not good
enough, then we can tweak whoever creates the Status object to fill it
out better as a separate patch.
I will look into this and see what is currently happening.