This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Error when there are no ports to launch a gdbserver on
ClosedPublic

Authored by DavidSpickett on Nov 17 2020, 7:52 AM.

Details

Summary

Previously if you did:
$ lldb-server platform --server <...> --min-gdbserver-port 12346
--max-gdbserver-port 12347
(meaning only use port 12346 for gdbservers)

Then tried to launch two gdbservers on the same connection,
the second one would return port 65535. Which is a real port
number but it actually means lldb-server didn't find one it was
allowed to use.

send packet: $qLaunchGDBServer;<...>
read packet: $pid:1919;port:12346;#c0
<...>
send packet: $qLaunchGDBServer;<...>
read packet: $pid:1927;port:65535;#c7

This situation should be an error even if port 65535 does happen
to be available on the current machine.

To fix this make PortMap it's own class within
GDBRemoteCommunicationServerPlatform.

This almost the same as the old typedef but for
GetNextAvailablePort() returning an llvm::Expected.
This means we have to handle not finding a port,
by returning an error packet.

Also add unit tests for this new PortMap class.

Diff Detail

Event Timeline

DavidSpickett created this revision.Nov 17 2020, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2020, 7:52 AM
DavidSpickett requested review of this revision.Nov 17 2020, 7:52 AM

I have found existing tests that launch an lldb-server with specific arguments, using an existing lldb-server platform to do it.

The ideal test would be:

  • run lldb-server with min port some known to be available port, max is that +1
  • connect the client
  • send qLaunchGDBServer twice, expect that the second one has an E response

What I can't work out how to do is decide which ports you'd need to use for the --min/max-gdbserver-port options. I couldn't find a way to predict what ports would be
available before you launch the lldb-server. Or guarantee that any ports that were available wouldn't get used up in the time between you finding them and launching the new lldb-server.

You have to give lldb-server at least one port number to use so giving it none and expecting error doesn't work.
Giving it a port you know is already used doesn't help either, since this code would still say it's available but then fail connect to it.

So presented without a test (and the most minimal code change). Ideas welcome though.

DavidSpickett edited the summary of this revision. (Show Details)Nov 17 2020, 8:01 AM
DavidSpickett edited the summary of this revision. (Show Details)

Yeah, I don't think it's possible to write a reliable end-to-end test for this feature. A unit test might be an option -- I don't know how hard it would be to mock the parts needed (starting a debug server). One could also consider refactoring this code to avoid the usage of a valid port number to represent the "no port" state (and while doing so, making the code easier to unit test).

While I would encourage you to try some of these things out, I wouldn't say that that's really required for this bugfix.

BTW, the --min/max-gdbserver-port is a constant source of bug reports (last one was last weel -- I'll have to reply to that one) and ideally I'd just do away with it. It just leaves a bad FTP taste in the mouth..

I hadn't considered using a unit test instead, I'll give that a go.

  • Move PortMap into a class we can unittest
  • Use llvm::Expected for GetNextAvailablePort
DavidSpickett added inline comments.Nov 24 2020, 7:55 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
30

Only the one in GDBRemoteCommunicationServerPlatform is/was used.

Looks good. Just some cosmetic details...

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
30

cool

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
74

auto, maybe

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
29–30

Add some comments to explain what these do.

lldb/tools/lldb-server/lldb-platform.cpp
234

Why the cast?

lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp
1

Please put this under unittests/Process/gdb-remote. The tests in here are a failed experiment, and I'll hopefully get rid of them soon.

24–25

ASSERT_THAT_EXPECTED(p1.GetNextAvailablePort(), llvm::HasValue(0));

33–34

Same, here, and further on...

  • Use auto for map find
  • Document PortMap functions
  • Remove unnecessary casts
  • Move tests to Process/gdb-remote
  • Cleanup Expected asserts

The behaviour of AssociatePortWithProcess is a bit
non obvious (that it can move a port to a different pid)
but I'm not inclined to change it now.

So I've just added a FIXME to note that this usage
will probably fail if we do later share the PortMap.

DavidSpickett marked 6 inline comments as done.Nov 26 2020, 4:06 AM
DavidSpickett added inline comments.
lldb/tools/lldb-server/lldb-platform.cpp
234

Me misreading a warning I think, removed here and below.

labath accepted this revision.Nov 29 2020, 11:57 PM

I was just asking for comments on the two constructors, but having them elsewhere is nice too.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
29–31

This looks like a class-level comment. Could you also say a few words about what the constructor does specifically.

This revision is now accepted and ready to land.Nov 29 2020, 11:57 PM
  • Document default constructor behaviour.
DavidSpickett marked an inline comment as done.Nov 30 2020, 2:10 AM
This revision was landed with ongoing or failed builds.Nov 30 2020, 2:19 AM
This revision was automatically updated to reflect the committed changes.