[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

2021-01-09 Thread walter erquinigo via Phabricator via lldb-commits
wallace updated this revision to Diff 315655.
wallace added a comment.

improve tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93951

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
  lldb/tools/lldb-vscode/CMakeLists.txt
  lldb/tools/lldb-vscode/FifoFiles.cpp
  lldb/tools/lldb-vscode/FifoFiles.h
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/JSONUtils.h
  lldb/tools/lldb-vscode/Options.td
  lldb/tools/lldb-vscode/RunInTerminal.cpp
  lldb/tools/lldb-vscode/RunInTerminal.h
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -384,12 +384,7 @@
 break;
   case lldb::eStateSuspended:
 break;
-  case lldb::eStateStopped: {
-if (g_vsc.waiting_for_run_in_terminal) {
-  g_vsc.waiting_for_run_in_terminal = false;
-  g_vsc.request_in_terminal_cv.notify_one();
-}
-  }
+  case lldb::eStateStopped:
 // Only report a stopped event if the process was not restarted.
 if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
   SendStdOutStdErr(process);
@@ -1441,47 +1436,64 @@
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
-void request_runInTerminal(const llvm::json::Object _request,
-   llvm::json::Object _response) {
-  // We have already created a target that has a valid "program" path to the
-  // executable. We will attach to the next process whose name matches that
-  // of the target's.
+llvm::Error request_runInTerminal(const llvm::json::Object _request) {
   g_vsc.is_attach = true;
   lldb::SBAttachInfo attach_info;
-  lldb::SBError error;
-  attach_info.SetWaitForLaunch(true, /*async*/ true);
-  g_vsc.target.Attach(attach_info, error);
 
-  llvm::json::Object reverse_request =
-  CreateRunInTerminalReverseRequest(launch_request);
+  llvm::Expected> comm_file_or_err =
+  CreateRunInTerminalCommFile();
+  if (!comm_file_or_err)
+return comm_file_or_err.takeError();
+  FifoFile _file = *comm_file_or_err.get();
+
+  RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.path);
+
+  llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
+  launch_request, g_vsc.debug_adaptor_path, comm_file.path);
   llvm::json::Object reverse_response;
   lldb_vscode::PacketStatus status =
   g_vsc.SendReverseRequest(reverse_request, reverse_response);
   if (status != lldb_vscode::PacketStatus::Success)
-error.SetErrorString("Process cannot be launched by IDE.");
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "Process cannot be launched by the IDE. %s",
+   comm_channel.GetLauncherError().c_str());
 
-  if (error.Success()) {
-// Wait for the attach stop event to happen or for a timeout.
-g_vsc.waiting_for_run_in_terminal = true;
-static std::mutex mutex;
-std::unique_lock locker(mutex);
-g_vsc.request_in_terminal_cv.wait_for(locker, std::chrono::seconds(10));
+  if (llvm::Expected pid = comm_channel.GetLauncherPid())
+attach_info.SetProcessID(*pid);
+  else
+return pid.takeError();
 
-auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
-if (attached_pid == LLDB_INVALID_PROCESS_ID)
-  error.SetErrorString("Failed to attach to a process");
-else
-  SendProcessEvent(Attach);
-  }
+  g_vsc.debugger.SetAsync(false);
+  lldb::SBError error;
+  g_vsc.target.Attach(attach_info, error);
 
-  if (error.Fail()) {
-launch_response["success"] = llvm::json::Value(false);
-EmplaceSafeString(launch_response, "message",
-  std::string(error.GetCString()));
-  } else {
-launch_response["success"] = llvm::json::Value(true);
-g_vsc.SendJSON(CreateEventObject("initialized"));
-  }
+  if (error.Fail())
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "Failed to attach to the target process. %s",
+   comm_channel.GetLauncherError().c_str());
+  // This will notify the runInTerminal launcher that we attached.
+  // We have to make this async, as the function won't return until the launcher
+  // resumes and reads the data.
+  std::future did_attach_message_success =
+  comm_channel.NotifyDidAttach();
+
+  // We just attached to the runInTerminal launcher, which was waiting to be
+  // attached. We now resume it, so it can receive the didAttach notification
+  

[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

2021-01-09 Thread walter erquinigo via Phabricator via lldb-commits
wallace updated this revision to Diff 315652.
wallace added a comment.

Addressed all comments:

- Now using a single file for all the communication. Did indeed simplified a 
lot of code
- Now adding timeouts to Send operations. This will help prevent the adaptor 
from undefinitely waiting for the launcher to read the data.
- I added more comments, specially in the part where the process attaches and 
resumes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93951

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
  lldb/tools/lldb-vscode/CMakeLists.txt
  lldb/tools/lldb-vscode/FifoFiles.cpp
  lldb/tools/lldb-vscode/FifoFiles.h
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/JSONUtils.h
  lldb/tools/lldb-vscode/Options.td
  lldb/tools/lldb-vscode/RunInTerminal.cpp
  lldb/tools/lldb-vscode/RunInTerminal.h
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -384,12 +384,7 @@
 break;
   case lldb::eStateSuspended:
 break;
-  case lldb::eStateStopped: {
-if (g_vsc.waiting_for_run_in_terminal) {
-  g_vsc.waiting_for_run_in_terminal = false;
-  g_vsc.request_in_terminal_cv.notify_one();
-}
-  }
+  case lldb::eStateStopped:
 // Only report a stopped event if the process was not restarted.
 if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
   SendStdOutStdErr(process);
@@ -1441,47 +1436,64 @@
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
-void request_runInTerminal(const llvm::json::Object _request,
-   llvm::json::Object _response) {
-  // We have already created a target that has a valid "program" path to the
-  // executable. We will attach to the next process whose name matches that
-  // of the target's.
+llvm::Error request_runInTerminal(const llvm::json::Object _request) {
   g_vsc.is_attach = true;
   lldb::SBAttachInfo attach_info;
-  lldb::SBError error;
-  attach_info.SetWaitForLaunch(true, /*async*/ true);
-  g_vsc.target.Attach(attach_info, error);
 
-  llvm::json::Object reverse_request =
-  CreateRunInTerminalReverseRequest(launch_request);
+  llvm::Expected> comm_file_or_err =
+  CreateRunInTerminalCommFile();
+  if (!comm_file_or_err)
+return comm_file_or_err.takeError();
+  FifoFile _file = *comm_file_or_err.get();
+
+  RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.path);
+
+  llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
+  launch_request, g_vsc.debug_adaptor_path, comm_file.path);
   llvm::json::Object reverse_response;
   lldb_vscode::PacketStatus status =
   g_vsc.SendReverseRequest(reverse_request, reverse_response);
   if (status != lldb_vscode::PacketStatus::Success)
-error.SetErrorString("Process cannot be launched by IDE.");
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "Process cannot be launched by the IDE. %s",
+   comm_channel.GetLauncherError().c_str());
 
-  if (error.Success()) {
-// Wait for the attach stop event to happen or for a timeout.
-g_vsc.waiting_for_run_in_terminal = true;
-static std::mutex mutex;
-std::unique_lock locker(mutex);
-g_vsc.request_in_terminal_cv.wait_for(locker, std::chrono::seconds(10));
+  if (llvm::Expected pid = comm_channel.GetLauncherPid())
+attach_info.SetProcessID(*pid);
+  else
+return pid.takeError();
 
-auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
-if (attached_pid == LLDB_INVALID_PROCESS_ID)
-  error.SetErrorString("Failed to attach to a process");
-else
-  SendProcessEvent(Attach);
-  }
+  g_vsc.debugger.SetAsync(false);
+  lldb::SBError error;
+  g_vsc.target.Attach(attach_info, error);
 
-  if (error.Fail()) {
-launch_response["success"] = llvm::json::Value(false);
-EmplaceSafeString(launch_response, "message",
-  std::string(error.GetCString()));
-  } else {
-launch_response["success"] = llvm::json::Value(true);
-g_vsc.SendJSON(CreateEventObject("initialized"));
-  }
+  if (error.Fail())
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "Failed to attach to the target process. %s",
+   comm_channel.GetLauncherError().c_str());
+  // This will notify the runInTerminal launcher that we attached.
+  // We have to make this async, 

[Lldb-commits] [PATCH] D93649: [lldb/Lua] add support for Lua function breakpoint

2021-01-09 Thread Pedro Tammela via Phabricator via lldb-commits
tammela added inline comments.



Comment at: 
lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_function_callback.test:15
+r
+# CHECK: 
+breakpoint command add -s lua -o "abc(frame, bp_loc, ...)"

JDevlieghere wrote:
> Can we unpack the SBStructuredData and check for `foo` or `123` in the output?
While I was doing this change, I noted that the `SBStructuredData` API for 
`GetStringValue` is quite odd.
For Lua, the auto-generated SWIG wrapper enforces code like this:
```
local result = ""
sd:GetStringValue(result, 3) -- 'sd' should be a value with at most 3 characters
```
This sort of API leaks all sorts of details to the Lua script. I think it could 
be improved to just return a `std::string` / `llvm::StringRef` object.

The SWIG wrapper also has some bugs for this class. For instance it's using 
`lua_pushnumber` where it should be `lua_pushinteger` for the `GetIntegerValue` 
method.

I will report this to the SWIG authors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93649

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


[Lldb-commits] [PATCH] D94357: [NFC] Add some additional, unconditional, logging to debugserver mostly related to app launching/attaching

2021-01-09 Thread Jason Molenda via Phabricator via lldb-commits
jasonmolenda created this revision.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
jasonmolenda requested review of this revision.

It can be tricky to find the important error messages when debugserver tries to 
launch or attach to a process in the system console log.  Add some new logging 
messages to give us clear before/after logging around the critical areas, so we 
can search other subsystems that might issue a console message about why the 
app launch failed.  These new logging messages are only intended to go to the 
system console, not stdout when running debugserver from the command line, 
unless logging is redirected somewhere else.

rdar://problem/67220442


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94357

Files:
  lldb/tools/debugserver/source/MacOSX/MachProcess.mm
  lldb/tools/debugserver/source/MacOSX/MachTask.mm
  lldb/tools/debugserver/source/RNBRemote.cpp
  lldb/tools/debugserver/source/debugserver.cpp

Index: lldb/tools/debugserver/source/debugserver.cpp
===
--- lldb/tools/debugserver/source/debugserver.cpp
+++ lldb/tools/debugserver/source/debugserver.cpp
@@ -583,29 +583,34 @@
   }
 
   if (set_events & RNBContext::event_proc_thread_exiting) {
+DNBLog("debugserver's process monitoring thread has exited.");
 mode = eRNBRunLoopModeExit;
   }
 
   if (set_events & RNBContext::event_read_thread_exiting) {
 // Out remote packet receiving thread exited, exit for now.
+DNBLog(
+"debugserver's packet communication to lldb has been shut down.");
 if (ctx.HasValidProcessID()) {
+  nub_process_t pid = ctx.ProcessID();
   // TODO: We should add code that will leave the current process
   // in its current state and listen for another connection...
   if (ctx.ProcessStateRunning()) {
 if (ctx.GetDetachOnError()) {
-  DNBLog("debugserver's event read thread is exiting, detaching "
- "from the inferior process.");
-  DNBProcessDetach(ctx.ProcessID());
+  DNBLog("debugserver has a valid PID %d, it is still running. "
+ "detaching from the inferior process.",
+ pid);
+  DNBProcessDetach(pid);
 } else {
-  DNBLog("debugserver's event read thread is exiting, killing the "
- "inferior process.");
-  DNBProcessKill(ctx.ProcessID());
+  DNBLog("debugserver killing the inferior process, pid %d.", pid);
+  DNBProcessKill(pid);
 }
   } else {
 if (ctx.GetDetachOnError()) {
-  DNBLog("debugserver's event read thread is exiting, detaching "
- "from the inferior process.");
-  DNBProcessDetach(ctx.ProcessID());
+  DNBLog("debugserver has a valid PID %d but it may no longer "
+ "be running, detaching from the inferior process.",
+ pid);
+  DNBProcessDetach(pid);
 }
   }
 }
@@ -1104,21 +1109,30 @@
   if (optarg && optarg[0]) {
 if (strcasecmp(optarg, "auto") == 0)
   g_launch_flavor = eLaunchFlavorDefault;
-else if (strcasestr(optarg, "posix") == optarg)
+else if (strcasestr(optarg, "posix") == optarg) {
+  DNBLog(
+  "[LaunchAttach] launch flavor is posix_spawn via cmdline option");
   g_launch_flavor = eLaunchFlavorPosixSpawn;
-else if (strcasestr(optarg, "fork") == optarg)
+} else if (strcasestr(optarg, "fork") == optarg)
   g_launch_flavor = eLaunchFlavorForkExec;
 #ifdef WITH_SPRINGBOARD
-else if (strcasestr(optarg, "spring") == optarg)
+else if (strcasestr(optarg, "spring") == optarg) {
+  DNBLog(
+  "[LaunchAttach] launch flavor is SpringBoard via cmdline option");
   g_launch_flavor = eLaunchFlavorSpringBoard;
+}
 #endif
 #ifdef WITH_BKS
-else if (strcasestr(optarg, "backboard") == optarg)
+else if (strcasestr(optarg, "backboard") == optarg) {
+  DNBLog("[LaunchAttach] launch flavor is BKS via cmdline option");
   g_launch_flavor = eLaunchFlavorBKS;
+}
 #endif
 #ifdef WITH_FBS
-else if (strcasestr(optarg, "frontboard") == optarg)
+else if (strcasestr(optarg, "frontboard") == optarg) {
+  DNBLog("[LaunchAttach] launch flavor is FBS via cmdline option");
   g_launch_flavor = eLaunchFlavorFBS;
+}
 #endif
 
 else {
@@ -1398,6 +1412,7 @@
 dup2(null, STDOUT_FILENO);
 dup2(null, STDERR_FILENO);
   } else if (g_applist_opt != 0) {
+DNBLog("debugserver running in --applist mode");
 // List all applications we are able to see
 std::string applist_plist;
 int err =