Page MenuHomePhabricator

Sanity check --max-gdbserver-port
ClosedPublic

Authored by jankratochvil on Mar 5 2019, 5:53 AM.

Details

Summary

In mail lldb-dev: Remote debugging a docker process user was confused by --min-gdbserver-port and --max-gdbserver-port options being ignored. I think there is even a bug that --max-gdbserver-port is upper exclusive limit (and not upper inclusive limit appropriate for max).
At least this patch should catch such mistake by an error message. The question is whether --max-gdbserver-port should not be changed to really be max and not max+1 but that would break backward compatibility.
Now the mail example does produce:

error: --min-gdbserver-port (5001) is not lower than --max-gdbserver-port (5001)

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jankratochvil created this revision.Mar 5 2019, 5:53 AM
labath added a subscriber: labath.Mar 5 2019, 6:22 AM

Perhaps it would be nice to write a test-case for that. It should be as simple as running not lldb-server --whatever and FileChecking the output.

There is no not as lldb-server still had+has exit code 0 during this error.
I have no idea what to put to the FIXME string there. Failure (with old lldb-server) looks as:

FAIL: LLDB :: Driver/TestGdbserverPort.test (8 of 1551)
******************** TEST 'LLDB :: Driver/TestGdbserverPort.test' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/jkratoch/redhat/llvm-monorepo-clangassert/bin/lldb-server platform p --server --listen :1234 --min-gdbserver-port 1234 --max-gdbserver-port 1234 2>&1 | /home/jkratoch/redhat/llvm-monorepo-clangassert/bin/FileCheck /home/jkratoch/redhat/llvm-monorepo/lldb/lit/Driver/TestGdbserverPort.test
--
Exit Code: 1

Command Output (stderr):
--
/home/jkratoch/redhat/llvm-monorepo/lldb/lit/Driver/TestGdbserverPort.test:2:10: error: CHECK: expected string not found in input
# CHECK: error: --min-gdbserver-port (1234) is not lower than --max-gdbserver-port (1234)
         ^
<stdin>:1:1: note: scanning from here
failed to listen: Address already in use
^
labath added inline comments.Mar 5 2019, 8:48 AM
lldb/lit/helper/toolchain.py
19 ↗(On Diff #189333)

For the "platform" mode, Darwin uses lldb-server like other platforms. So this should just be "lldb-server platform" unconditionally.

jankratochvil marked an inline comment as done.

LGTM fwiw I think the --min-gdbserver-port and --max-gdbserver-port options were primarily/only used for iOS etc testsuite runs, where a fixed set of ports is relayed between the devices. I was having some problems with lldb-server in platform mode on these devices and thought it would be nice to have a standalone implementation of the platform protocol packets, so I wrote one a couple months ago and that's what we're using now for this environment. I should probably upstream it, I think it would work on linux too, it doesn't use any of lldb/llvm so it has several areas that are primitive because I was going for "good enough" and moving on, e.g. it has a dumb logging class and the networking could use some work, etc. I want to keep this completely free of lldb/llvm dependencies so it's a little bit of an oddball.

labath accepted this revision.Mar 6 2019, 3:18 AM

Looks good. Thank you for fixing this. I'd consider moving the test to lit/lldb-server (new folder) to mirror the location of the code being tested. (Driver refers to the main lldb exe).

This revision is now accepted and ready to land.Mar 6 2019, 3:18 AM

I'd consider moving the test to lit/lldb-server (new folder)

Don't you mean lit/LldbServer? As directories there are UpperCamelCased.

I'd consider moving the test to lit/lldb-server (new folder)

Don't you mean lit/LldbServer? As directories there are UpperCamelCased.

There's a tools folder (lit/tools) where this would fit. It's lowercase, possibly to match llvm's test/tools directory?

This revision was automatically updated to reflect the committed changes.

There's a tools folder (lit/tools) where this would fit. It's lowercase, possibly to match llvm's test/tools directory?

Yes, lit/tools/lldb-server/TestGdbserverPort.test looks as the right location.
Thanks for all the comments.

This caused a failure on the Windows bot:

Checked in rL355579 to skip the test on Windows as lldb-server is not being built on Windows.

There's a tools folder (lit/tools) where this would fit. It's lowercase, possibly to match llvm's test/tools directory?

Yes, lit/tools/lldb-server/TestGdbserverPort.test looks as the right location.

Looks good to me.