[Lldb-commits] [PATCH] D138164: [LLDB][Android] Fix Android serial number handling

Wed Nov 16 21:56:48 PST 2022

clayborg added a comment.

Just wanted to let you know that the "platform connect" parameters are not fixed. Each platform can make their own arguments. The Android platform currently expects a URL that will get forwarded to an internal Connection class that has registered a connection URL prefix ("unix-abstract-connect" in this case). But it doesn't have to be this way.

If the "unix-abstract-connect://emulator-5554/data/local/tmp/debug.sock" is not actually just being passed to the ConnectionFileDescriptor::Connect(...) function in lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp then we might want just modify the "platform connect" in the android platform to accept extra options since each Platform plug-in could have any number of options needed to connect. So instead of adding the device ID to the URL we could use a --device option. Currently we have:

  (lldb) platform connect unix-abstract-connect://emulator-5554/data/local/tmp/debug.sock

But this could be:

  platform connect --device emulator-5554 unix-abstract-connect://data/local/tmp/debug.sock

Same kind of thing for the TCP address. Not sure if we would specify this with "--device 127.0.0.1:5556" or if we would add a "--tcp 127.0.0.1:5556" option?

So I just wanted to let you know that we don't have to fit all of the connection information into a single URL. I am also not sure if the current android platform code was already doing this kind of thing? The "unix-abstract-connect" is a url that should forward the contents directly to a connection plug-in where we can have connection subclasses register connection URL prefixes that any plug-in can use. So I am not sure if adding the device name to this URL ("unix-abstract-connect://data/local/tmp/debug.sock") is the right way to go.

Let me know what you think and if I have misunderstood anything of how things currently work versus how this patch is modifying things to work.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138164