[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 created this revision.
Herald added a subscriber: danielkiss.
Herald added a project: All.
mark2185 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The local ports for `platform connect` and `attach` were always random, this 
allows the user to configure them.
This is useful for debugging a truly remote android (when the android in 
question is connected to a remote server).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136465

Files:
  lldb/docs/use/remote.rst
  lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
  lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h

Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
@@ -49,7 +49,8 @@
 
   void DeleteForwardPort(lldb::pid_t pid);
 
-  Status MakeConnectURL(const lldb::pid_t pid, const uint16_t remote_port,
+  Status MakeConnectURL(const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port,
 llvm::StringRef remote_socket_name,
 std::string &connect_url);
 
Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -90,8 +90,14 @@
 
   Log *log = GetLog(LLDBLog::Platform);
 
-  auto error =
-  MakeConnectURL(pid, remote_port, socket_name.c_str(), connect_url);
+  uint16_t local_port = 0;
+  const char *gdbstub_port = std::getenv("ANDROID_PLATFORM_LOCAL_GDB_PORT");
+  if (gdbstub_port) {
+local_port = std::stoi(gdbstub_port);
+  }
+
+  auto error = MakeConnectURL(pid, local_port, remote_port, socket_name.c_str(),
+  connect_url);
   if (error.Success() && log)
 LLDB_LOGF(log, "gdbserver connect URL: %s", connect_url.c_str());
 
@@ -126,10 +132,16 @@
   else if (parsed_url->scheme == "unix-abstract-connect")
 m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
 
+  uint16_t local_port = 0;
+  const char *platform_local_port = std::getenv("ANDROID_PLATFORM_LOCAL_PORT");
+  if (platform_local_port) {
+local_port = std::stoi(platform_local_port);
+  }
+
   std::string connect_url;
-  auto error =
-  MakeConnectURL(g_remote_platform_pid, parsed_url->port.value_or(0),
- parsed_url->path, connect_url);
+  auto error = MakeConnectURL(g_remote_platform_pid, local_port,
+  parsed_url->port.value_or(0), parsed_url->path,
+  connect_url);
 
   if (error.Fail())
 return error;
@@ -170,11 +182,29 @@
 }
 
 Status PlatformAndroidRemoteGDBServer::MakeConnectURL(
-const lldb::pid_t pid, const uint16_t remote_port,
-llvm::StringRef remote_socket_name, std::string &connect_url) {
+const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port, llvm::StringRef remote_socket_name,
+std::string &connect_url) {
   static const int kAttempsNum = 5;
 
   Status error;
+
+  auto forward = [&](const uint16_t local, const uint16_t remote) {
+error = ForwardPortWithAdb(local, remote, remote_socket_name,
+   m_socket_namespace, m_device_id);
+if (error.Success()) {
+  m_port_forwards[pid] = local;
+  std::ostringstream url_str;
+  url_str << "connect://127.0.0.1:" << local;
+  connect_url = url_str.str();
+}
+return error;
+  };
+
+  if (local_port != 0) {
+return forward(local_port, remote_port);
+  }
+
   // There is a race possibility that somebody will occupy a port while we're
   // in between FindUnusedPort and ForwardPortWithAdb - adding the loop to
   // mitigate such problem.
@@ -184,13 +214,7 @@
 if (error.Fail())
   return error;
 
-error = ForwardPortWithAdb(local_port, remote_port, remote_socket_name,
-   m_socket_namespace, m_device_id);
-if (error.Success()) {
-  m_port_forwards[pid] = local_port;
-  std::ostringstream url_str;
-  url_str << "connect://127.0.0.1:" << local_port;
-  connect_url = url_str.str();
+if (forward(local_port, remote_port).Success()) {
   break;
 }
   }
@@ -216,7 +240,7 @@
   }
 
   std::string new_connect_url;
-  error = MakeConnectURL(s_remote_gdbserver_fake_pid--,
+  error = MakeConnectURL(s_remote_gdbserver_fake_pid--, 0,
  parsed_url->port.value_or(0), parsed_url->path,
  new_connect_url);
   if (error.Fail())
Index: lldb/docs/use/remote.rst
=

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 added a comment.

In D136465#3878581 , @DavidSpickett 
wrote:

> Does this account for the situation where you spawn many gdbserver from the 
> platform and therefore need more ports? Does it even need to? (just guessing 
> that that could have been the reason for selecting random ports, though not 
> for the platform)

To be honest, I don't know. In all of my testing of practical use cases (i.e. 
launching an app and debugging it either through `CodeLLDB/vscode-lldb` or 
`Android Studio`), I've only ever seen two ports. One for the platform (when 
running `platform connect`), and one for `gdb` (when trying to `attach` to a 
`PID`).

`git blame` points to this , as previously it 
was using the port specified when launching `lldb-server` for the `local` port, 
but it turns out it's better to have a random one and then forward it. And it 
makes sense, since the device you're trying to debug is //very likely// plugged 
into the same machine as you're running `ADB` on, which is perfect for 
//local// development.

But that wasn't good enough for the use case I have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 added a comment.

In D136465#3878640 , @DavidSpickett 
wrote:

> Ok, so this change means that the randomisation still happens, unless you 
> override it with these environment variables? This seems like a good way to 
> do it.

Exactly, this is completely opt-in. This approach with `environment` variables 
was easy enough, and it was used in other things related to `adb` so it seemed 
fitting. Maybe it would be better to set it through `platform settings`, but 
I'd rather have someone chip in on that.

Also the burden of making sure the ports are //actually// free is on the user, 
but I'm guessing the user is aware of this since they are using custom ports.

> Where do these env vars need to be set? On the local machine, on the 
> lldb-server machine, on the device?

These are **client-side** variables, since that is where the user is launching 
`lldb` from. `adb` will only get a request `forward tcp: 
tcp:` instead of `forward tcp: tcp:`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 added a comment.

In D136465#3880809 , @clayborg wrote:

> Is there any way to test this?

Through the test suite? I couldn't find //any// `android platform` related 
tests, and this would require `adb` to be running in the background, so I'm not 
sure how to approach it.

As for manually testing, three shells are needed (one for `lldb-server`, one 
for `lldb`, and one to check the ports).

  (shell-1) $> adb push /path/to/correct/cpu/arch/lldb-server /data/local/tmp   
# (android studio comes with prebuilt servers)
  (shell-1) $> adb shell /data/local/tmp/lldb-server platform --listen "*:"



  (shell-2) $> ANDROID_PLATFORM_LOCAL_PORT= lldb
  (lldb) $> platform select remote-android
  (lldb) $> platform connect connect://localhost:



  (shell-3) $> adb forward --list
   tcp: tcp:

If one were to omit the `ANDROID_PLATFORM_LOCAL_PORT` variable, the forwarded 
port will be random.

As for testing the `ANDROID_PLATFORM_LOCAL_GDB_PORT`, one would need to:

- install an app on the device
- run the `lldb-server` as that app ( `adb shell run-as com.full.package.App 
./lldb-server platform...` ) so it can attach to its `PID`
- after `platform connect` run `attach `
- `adb forward --list` will now show two forwarded ports
  - the `ANDROID_PLATFORM_LOCAL_PORT` from before
  - the  `ANDROID_PLATFORM_LOCAL_GDB_PORT`, forwarded to some port on the 
device (that destination port is also random, unless `lldb-server` is started 
with the `--gdbserver-port` argument, or `--min-gdbserver-port` and 
`--max-gdbserver-port` that limit its possible range)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 updated this revision to Diff 470381.
mark2185 added a comment.

Remove {} for single line if statements per LLVM coding guidelines


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

Files:
  lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp


Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -92,9 +92,8 @@
 
   uint16_t local_port = 0;
   const char *gdbstub_port = std::getenv("ANDROID_PLATFORM_LOCAL_GDB_PORT");
-  if (gdbstub_port) {
+  if (gdbstub_port)
 local_port = std::stoi(gdbstub_port);
-  }
 
   auto error = MakeConnectURL(pid, local_port, remote_port, 
socket_name.c_str(),
   connect_url);
@@ -134,9 +133,8 @@
 
   uint16_t local_port = 0;
   const char *platform_local_port = std::getenv("ANDROID_PLATFORM_LOCAL_PORT");
-  if (platform_local_port) {
+  if (platform_local_port)
 local_port = std::stoi(platform_local_port);
-  }
 
   std::string connect_url;
   auto error = MakeConnectURL(g_remote_platform_pid, local_port,
@@ -201,9 +199,8 @@
 return error;
   };
 
-  if (local_port != 0) {
+  if (local_port != 0)
 return forward(local_port, remote_port);
-  }
 
   // There is a race possibility that somebody will occupy a port while we're
   // in between FindUnusedPort and ForwardPortWithAdb - adding the loop to
@@ -214,9 +211,8 @@
 if (error.Fail())
   return error;
 
-if (forward(local_port, remote_port).Success()) {
+if (forward(local_port, remote_port).Success())
   break;
-}
   }
 
   return error;


Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -92,9 +92,8 @@
 
   uint16_t local_port = 0;
   const char *gdbstub_port = std::getenv("ANDROID_PLATFORM_LOCAL_GDB_PORT");
-  if (gdbstub_port) {
+  if (gdbstub_port)
 local_port = std::stoi(gdbstub_port);
-  }
 
   auto error = MakeConnectURL(pid, local_port, remote_port, socket_name.c_str(),
   connect_url);
@@ -134,9 +133,8 @@
 
   uint16_t local_port = 0;
   const char *platform_local_port = std::getenv("ANDROID_PLATFORM_LOCAL_PORT");
-  if (platform_local_port) {
+  if (platform_local_port)
 local_port = std::stoi(platform_local_port);
-  }
 
   std::string connect_url;
   auto error = MakeConnectURL(g_remote_platform_pid, local_port,
@@ -201,9 +199,8 @@
 return error;
   };
 
-  if (local_port != 0) {
+  if (local_port != 0)
 return forward(local_port, remote_port);
-  }
 
   // There is a race possibility that somebody will occupy a port while we're
   // in between FindUnusedPort and ForwardPortWithAdb - adding the loop to
@@ -214,9 +211,8 @@
 if (error.Fail())
   return error;
 
-if (forward(local_port, remote_port).Success()) {
+if (forward(local_port, remote_port).Success())
   break;
-}
   }
 
   return error;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 marked 2 inline comments as done.
mark2185 added a comment.

Did I just overwrite the initial commit with a new one, instead of just 
creating a diff based on the two comments?

I'm terribly sorry, should I just squash my two commits and run `arc diff 
--update=D136465` again?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 updated this revision to Diff 470421.
mark2185 added a comment.

Make remote-android local ports configurable

The local ports for `platform connect` and `attach` were always random, this 
allows the user to configure them.
This is useful for debugging a truly remote android (when the android in 
question is connected to a remote server).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

Files:
  lldb/docs/use/remote.rst
  lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
  lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h

Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
@@ -49,7 +49,8 @@
 
   void DeleteForwardPort(lldb::pid_t pid);
 
-  Status MakeConnectURL(const lldb::pid_t pid, const uint16_t remote_port,
+  Status MakeConnectURL(const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port,
 llvm::StringRef remote_socket_name,
 std::string &connect_url);
 
Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -90,8 +90,13 @@
 
   Log *log = GetLog(LLDBLog::Platform);
 
-  auto error =
-  MakeConnectURL(pid, remote_port, socket_name.c_str(), connect_url);
+  uint16_t local_port = 0;
+  const char *gdbstub_port = std::getenv("ANDROID_PLATFORM_LOCAL_GDB_PORT");
+  if (gdbstub_port)
+local_port = std::stoi(gdbstub_port);
+
+  auto error = MakeConnectURL(pid, local_port, remote_port, socket_name.c_str(),
+  connect_url);
   if (error.Success() && log)
 LLDB_LOGF(log, "gdbserver connect URL: %s", connect_url.c_str());
 
@@ -126,10 +131,15 @@
   else if (parsed_url->scheme == "unix-abstract-connect")
 m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
 
+  uint16_t local_port = 0;
+  const char *platform_local_port = std::getenv("ANDROID_PLATFORM_LOCAL_PORT");
+  if (platform_local_port)
+local_port = std::stoi(platform_local_port);
+
   std::string connect_url;
-  auto error =
-  MakeConnectURL(g_remote_platform_pid, parsed_url->port.value_or(0),
- parsed_url->path, connect_url);
+  auto error = MakeConnectURL(g_remote_platform_pid, local_port,
+  parsed_url->port.value_or(0), parsed_url->path,
+  connect_url);
 
   if (error.Fail())
 return error;
@@ -170,11 +180,28 @@
 }
 
 Status PlatformAndroidRemoteGDBServer::MakeConnectURL(
-const lldb::pid_t pid, const uint16_t remote_port,
-llvm::StringRef remote_socket_name, std::string &connect_url) {
+const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port, llvm::StringRef remote_socket_name,
+std::string &connect_url) {
   static const int kAttempsNum = 5;
 
   Status error;
+
+  auto forward = [&](const uint16_t local, const uint16_t remote) {
+error = ForwardPortWithAdb(local, remote, remote_socket_name,
+   m_socket_namespace, m_device_id);
+if (error.Success()) {
+  m_port_forwards[pid] = local;
+  std::ostringstream url_str;
+  url_str << "connect://127.0.0.1:" << local;
+  connect_url = url_str.str();
+}
+return error;
+  };
+
+  if (local_port != 0)
+return forward(local_port, remote_port);
+
   // There is a race possibility that somebody will occupy a port while we're
   // in between FindUnusedPort and ForwardPortWithAdb - adding the loop to
   // mitigate such problem.
@@ -184,15 +211,8 @@
 if (error.Fail())
   return error;
 
-error = ForwardPortWithAdb(local_port, remote_port, remote_socket_name,
-   m_socket_namespace, m_device_id);
-if (error.Success()) {
-  m_port_forwards[pid] = local_port;
-  std::ostringstream url_str;
-  url_str << "connect://127.0.0.1:" << local_port;
-  connect_url = url_str.str();
+if (forward(local_port, remote_port).Success())
   break;
-}
   }
 
   return error;
@@ -216,7 +236,7 @@
   }
 
   std::string new_connect_url;
-  error = MakeConnectURL(s_remote_gdbserver_fake_pid--,
+  error = MakeConnectURL(s_remote_gdbserver_fake_pid--, 0,
  parsed_url->port.value_or(0), parsed_url->path,
  new_connect_url);
   if (error.Fail())
Index: lldb/docs/use/remote.rst
===
--- lldb/docs/use/remote.r

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 added a comment.

In D136465#3884047 , @clayborg wrote:

> Looks ok to me. I don't do Android stuff on a daily basis. As long as the old 
> way of connecting still works I think this is ok.

Great! I'd just like to note that I do not have commit access, per the guide's 
instructions .

> It would be great to get more stuff in the Android remove platform connection 
> working at some point. Users still have to manually install things and a lot 
> of the work gets done by the Android Studio IDE kind of like Xcode does. I 
> would love to us add some features in the future:
>
> - allow a lldb_private::Target to have an application bundle FileSpec, which 
> could be pointed to a .apk file for Android and a .ipa file for iOS
> - Implement PlatformAndroid::LaunchProcess
>   - implement "Status PlatformAndroid::Install(const FileSpec &src, const 
> FileSpec &dst)" and handle APK files from target by doing "adb install" if 
> needed
>   - Have PlatformAndroid::LaunchProcess do everything that is needed to debug 
> the app
> - start lldb-server for the new process via the connect to the 
> "lldb-server platform" process
> - forward any ports needed
>
> It would be great at some point to be able to do something like:
>
>   (lldb) platform select remote-android
>   (lldb) platform connect ...
>   (lldb) target create --app-bundle /path/to/foo.apk
>   (lldb) run
>
> And have PlatformAndroid take care of everything for us.

That does sound pretty useful, and even Android Studio would benefit from it 
since currently it just pushes a shell script that launches the `lldb-server`. 
I'll see what I can do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D136465: Make remote-android local ports configurable

[Luka Markušić via Phabricator via lldb-commits]

mark2185 added a comment.

In D136465#3884755 , @DavidSpickett 
wrote:

>> Great! I'd just like to note that I do not have commit access, per the 
>> guide's instructions.
>
> What name/email address do you want on the commit?

Luka Markušić (markusicl...@gmail.com)

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136465/new/

https://reviews.llvm.org/D136465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits