[Lldb-commits] [PATCH] D124814: Fix debugserver translation check

[Alexandre Perez via Phabricator via lldb-commits]

aperez created this revision.
Herald added subscribers: pengfei, kristof.beyls.
Herald added a project: All.
aperez edited the summary of this revision.
aperez added a project: LLDB.
Herald added a subscriber: JDevlieghere.
aperez added reviewers: clayborg, jasonmolenda.
aperez published this revision for review.
aperez added a comment.
Herald added a subscriber: lldb-commits.

I did not add unit tests because I would need to debug an x86_64 binary using 
an x86_64 debugserver on arm64, and was not sure if the test infrastructure 
allowed for that.

I did run a few manual tests which I'm including in this comment. I compiled 
and used a `Darwin-x86_64` debugserver with the patch from this diff applied. 
`x86_64` processes are now handed off to the translated debugserver:

  $ lldb main_x86_64
  (lldb) target create "main_x86_64"
  Current executable set to '/Users/alexandreperez/temp/main_x86_64' (x86_64).
  (lldb) b main
  Breakpoint 1: where = main_x86_64`main + 11 at main.cpp:2:3, address = 
0x00013fab
  (lldb) r
  Process 80635 launched: '/Users/alexandreperez/temp/main_x86_64' (x86_64)
  Process 80635 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  frame #0: 0x00013fab main_x86_64`main at main.cpp:2:3
 1  int main() {
  -> 2return 0;
 3  }
  (lldb)

Verified that the `arm64` binary fail to attach using the patched 
`Darwin-x86_64` debugserver, as expected:

  $ lldb main_arm64
  (lldb) target create "main_arm64"
  Current executable set to '/Users/alexandreperez/temp/main_arm64' (arm64).
  (lldb) r
  error: process exited with status -1 (debugserver is x86_64 binary running in 
translation, attach failed.)
  (lldb)

Using the `LLDB_DEBUGSERVER_PATH` environment variable prevents from handing 
off to the translated debugserver, as expected:

  $ LLDB_DEBUGSERVER_PATH="/path/to/patched/debugserver" lldb main_x86_64
  (lldb) target create "main_x86_64"
  Current executable set to '/Users/alexandreperez/temp/main_x86_64' (x86_64).
  (lldb) r
  error: process exited with status -1 (debugserver is x86_64 binary running in 
translation, attach failed.)
  (lldb)


Currently, debugserver has a test to check if it was launched in
translation. The intent was to cover the case where an x86_64
debugserver attempts to control an arm64/arm64e process, returning
an error. However, this check also covers the case where users
are attaching to an x86_64 process, exiting out before attempting
to hand off control to the translated debugserver at
`/Library/Apple/usr/libexec/oah/debugserver`.

This diff delays the debugserver translation check until after
determining whether to hand off control to 
`/Library/Apple/usr/libexec/oah/debugserver`. Only when the 
process is not translated and thus has not been handed off do we
check if the debugserver is translated, erroring out in that case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124814

Files:
  lldb/tools/debugserver/source/DNB.cpp
  lldb/tools/debugserver/source/DNBDefs.h
  lldb/tools/debugserver/source/RNBRemote.cpp


Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3753,17 +3753,6 @@
 char err_str[1024] = {'\0'};
 std::string attach_name;
 
-if (DNBDebugserverIsTranslated()) {
-  DNBLogError("debugserver is x86_64 binary running in translation, attach 
"
-  "failed.");
-  std::string return_message = "E96;";
-  return_message +=
-  cstring_to_asciihex_string("debugserver is x86_64 binary running in "
- "translation, attached failed.");
-  SendPacket(return_message);
-  return rnb_err;
-}
-
 if (strstr(p, "vAttachWait;") == p) {
   p += strlen("vAttachWait;");
   if (!GetProcessNameFrom_vAttach(p, attach_name)) {
@@ -3823,6 +3812,17 @@
   return HandlePacket_UNIMPLEMENTED(p);
 }
 
+if (attach_pid == INVALID_NUB_PROCESS_ARCH) {
+  DNBLogError("debugserver is x86_64 binary running in translation, attach 
"
+  "failed.");
+  std::string return_message = "E96;";
+  return_message +=
+  cstring_to_asciihex_string("debugserver is x86_64 binary running in "
+ "translation, attach failed.");
+  SendPacket(return_message.c_str());
+  return rnb_err;
+}
+
 if (attach_pid != INVALID_NUB_PROCESS) {
   if (m_ctx.ProcessID() != attach_pid)
 m_ctx.SetProcessID(attach_pid);
Index: lldb/tools/debugserver/source/DNBDefs.h
===
--- lldb/tools/debugserver/source/DNBDefs.h
+++ lldb/tools/debugserver/source/DNBDefs.h
@@ -54,6 +54,7 @@
 typedef uint32_t nub_bool_t;
 
 #define INVALID_NUB_PROCESS ((nub_process_t)0)
+#define INVALID_

[Lldb-commits] [PATCH] D124814: Fix debugserver translation check

[Alexandre Perez via Phabricator via lldb-commits]

aperez added a comment.

In D124814#3487179 , @jasonmolenda 
wrote:

> Ah, I see what you're doing.  You've got a shell running x86_64 or something 
> (maybe started lldb as x86_64) so everything that is spawned from that is 
> x86_64 -- debugserver and the inferior process -- and so you hit this.  let 
> me look more closely later tonight, but I didn't take that scenario into 
> account.  Normally when people are running debugserver as x86_64 it's because 
> they unintentionally ran a parent as x86_64 and are paying an unintended perf 
> hit across the debug session and part of this error reporting was to say 
> "hey, you probably didn't mean to do this".

In our case we are currently distributing an x86_64-only version of the 
toolchain (including lldb and debugserver) across many machines. Some of these 
machines are m1 laptops, and we started observing failures to attach on these 
machines once 0c443e92d3b9 
 was 
included.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124814

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

[Lldb-commits] [PATCH] D124814: Fix debugserver translation check

[Alexandre Perez via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGeb3136f022b3: Fix debugserver translation check (authored by 
aperez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124814

Files:
  lldb/tools/debugserver/source/DNB.cpp
  lldb/tools/debugserver/source/DNBDefs.h
  lldb/tools/debugserver/source/RNBRemote.cpp


Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3753,17 +3753,6 @@
 char err_str[1024] = {'\0'};
 std::string attach_name;
 
-if (DNBDebugserverIsTranslated()) {
-  DNBLogError("debugserver is x86_64 binary running in translation, attach 
"
-  "failed.");
-  std::string return_message = "E96;";
-  return_message +=
-  cstring_to_asciihex_string("debugserver is x86_64 binary running in "
- "translation, attached failed.");
-  SendPacket(return_message);
-  return rnb_err;
-}
-
 if (strstr(p, "vAttachWait;") == p) {
   p += strlen("vAttachWait;");
   if (!GetProcessNameFrom_vAttach(p, attach_name)) {
@@ -3823,6 +3812,17 @@
   return HandlePacket_UNIMPLEMENTED(p);
 }
 
+if (attach_pid == INVALID_NUB_PROCESS_ARCH) {
+  DNBLogError("debugserver is x86_64 binary running in translation, attach 
"
+  "failed.");
+  std::string return_message = "E96;";
+  return_message +=
+  cstring_to_asciihex_string("debugserver is x86_64 binary running in "
+ "translation, attach failed.");
+  SendPacket(return_message.c_str());
+  return rnb_err;
+}
+
 if (attach_pid != INVALID_NUB_PROCESS) {
   if (m_ctx.ProcessID() != attach_pid)
 m_ctx.SetProcessID(attach_pid);
Index: lldb/tools/debugserver/source/DNBDefs.h
===
--- lldb/tools/debugserver/source/DNBDefs.h
+++ lldb/tools/debugserver/source/DNBDefs.h
@@ -54,6 +54,7 @@
 typedef uint32_t nub_bool_t;
 
 #define INVALID_NUB_PROCESS ((nub_process_t)0)
+#define INVALID_NUB_PROCESS_ARCH ((nub_process_t)-1)
 #define INVALID_NUB_THREAD ((nub_thread_t)0)
 #define INVALID_NUB_WATCH_ID ((nub_watch_t)0)
 #define INVALID_NUB_HW_INDEX UINT32_MAX
Index: lldb/tools/debugserver/source/DNB.cpp
===
--- lldb/tools/debugserver/source/DNB.cpp
+++ lldb/tools/debugserver/source/DNB.cpp
@@ -477,6 +477,10 @@
 }
   }
 
+  if (DNBDebugserverIsTranslated()) {
+return INVALID_NUB_PROCESS_ARCH;
+  }
+
   pid_t pid = INVALID_NUB_PROCESS;
   MachProcessSP processSP(new MachProcess);
   if (processSP.get()) {


Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -3753,17 +3753,6 @@
 char err_str[1024] = {'\0'};
 std::string attach_name;
 
-if (DNBDebugserverIsTranslated()) {
-  DNBLogError("debugserver is x86_64 binary running in translation, attach "
-  "failed.");
-  std::string return_message = "E96;";
-  return_message +=
-  cstring_to_asciihex_string("debugserver is x86_64 binary running in "
- "translation, attached failed.");
-  SendPacket(return_message);
-  return rnb_err;
-}
-
 if (strstr(p, "vAttachWait;") == p) {
   p += strlen("vAttachWait;");
   if (!GetProcessNameFrom_vAttach(p, attach_name)) {
@@ -3823,6 +3812,17 @@
   return HandlePacket_UNIMPLEMENTED(p);
 }
 
+if (attach_pid == INVALID_NUB_PROCESS_ARCH) {
+  DNBLogError("debugserver is x86_64 binary running in translation, attach "
+  "failed.");
+  std::string return_message = "E96;";
+  return_message +=
+  cstring_to_asciihex_string("debugserver is x86_64 binary running in "
+ "translation, attach failed.");
+  SendPacket(return_message.c_str());
+  return rnb_err;
+}
+
 if (attach_pid != INVALID_NUB_PROCESS) {
   if (m_ctx.ProcessID() != attach_pid)
 m_ctx.SetProcessID(attach_pid);
Index: lldb/tools/debugserver/source/DNBDefs.h
===
--- lldb/tools/debugserver/source/DNBDefs.h
+++ lldb/tools/debugserver/source/DNBDefs.h
@@ -54,6 +54,7 @@
 typedef uint32_t nub_bool_t;
 
 #define INVALID_NUB_PROCESS ((nub_process_t)0)
+#define INVALID_NUB_PROCESS_ARCH ((nub_process_t)-1)
 #define INVALID_NUB_THREAD ((nub_thread_t)0)
 #define INVALID_NUB_WATCH_ID ((nub_watch_t)0)
 #define INVALID_NUB_HW_INDEX UINT32_MAX
Index: lldb/tools/debugserver/source/DNB.cpp
=