Sanity check --max-gdbserver-port

[jankratochvil]

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)

[labath]

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.

[jankratochvil]

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]

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.

[jasonmolenda]

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]

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).

[jankratochvil]

In D58962#1419613, @labath wrote:

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

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

[JDevlieghere]

In D58962#1419628, @jankratochvil wrote:

In D58962#1419613, @labath wrote:

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?

[jankratochvil]

In D58962#1420137, @JDevlieghere wrote:

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.

[stella.stamenova]

This caused a failure on the Windows bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/2356

[jankratochvil]

In D58962#1420626, @stella.stamenova wrote:

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.

[labath]

In D58962#1420529, @jankratochvil wrote:

In D58962#1420137, @JDevlieghere wrote:

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.

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

case '?':
       break;
     }
   }
 
   if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
     return -1;
 
   // Make a port map for a port range that was specified.
-  if (min_gdbserver_port < max_gdbserver_port) {
+  if (min_gdbserver_port && min_gdbserver_port < max_gdbserver_port) {
     for (uint16_t port = min_gdbserver_port; port < max_gdbserver_port; ++port)
       gdbserver_portmap[port] = LLDB_INVALID_PROCESS_ID;
-  } else if (min_gdbserver_port != max_gdbserver_port) {
-    fprintf(stderr, "error: --min-gdbserver-port (%u) is greater than "
+  } else if (min_gdbserver_port || max_gdbserver_port) {
+    fprintf(stderr, "error: --min-gdbserver-port (%u) is not lower than "
                     "--max-gdbserver-port (%u)\n",
             min_gdbserver_port, max_gdbserver_port);
     option_error = 3;
   }
 
   // Print usage and exit if no listening port is specified.
   if (listen_host_port.empty())
     show_usage = true;