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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Thanks for your efforts here, very much appreciated.


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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Landed as 
https://github.com/llvm/llvm-project/commit/1e210abf9925ad08fb7c79894b4ec5ef8f0ef173.


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

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e210abf9925: [LLDB] Make remote-android local ports 
configurable (authored by mark2185, committed by DavidSpickett).

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 _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 _url) {
+const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port, llvm::StringRef remote_socket_name,
+std::string _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.rst
+++ lldb/docs/use/remote.rst
@@ -135,6 +135,11 @@
 commands: get-file, put-file, mkdir, etc. The environment can be prepared
 further using the platform shell 

[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

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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> 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?


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#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 , const 
> FileSpec )" 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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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.

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 , const 
FileSpec )" 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.


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 _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 _url) {
+const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port, llvm::StringRef remote_socket_name,
+std::string _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.rst
+++ 

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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Yes, somehow, and yes just update again.

Happens to us all, I have learned to not try to be clever with arc and just 
keep one commit per review.

Tip: if you look in the "Revision Contents" box there is a "history" and you 
can diff between versions of the patch, if you're ever not sure of what 
actually changed.


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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Is there any way to test this?




Comment at: 
lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:95-97
+  if (gdbstub_port) {
+local_port = std::stoi(gdbstub_port);
+  }

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



Comment at: 
lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:137-139
+  if (platform_local_port) {
+local_port = std::stoi(platform_local_port);
+  }

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

___
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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

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.

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


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#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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I don't have the expertise to review, but have one question.

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)


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 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 _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 _url) {
+const lldb::pid_t pid, const uint16_t local_port,
+const uint16_t remote_port, llvm::StringRef remote_socket_name,
+std::string _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
===
---