[vscode] set default values for terminateDebuggee for the disconnect request

[wallace]

Summary

Recently I've noticed that VSCode sometimes doesn't send the terminateDebuggee flag within the disconnectRequest,
even though lldb-vscode sets the terminateDebuggee capability correctly.
This has been causing that inferiors don't die after the debug session ends, and many users have reported issues because of this.

An easy way to mitigate this is to set better default values for the terminateDebuggee field in the disconnect request.
I'm assuming that for a launch request, the default will be true, and for attach it'll be false.

[clayborg]

So the main question is why was lldb-vscode not exiting. Do we know why? Some thread wasn't exiting I would guess, but usually as long as the main thread exits, all other threads will just be terminated. Our main thread should be the loop that waits for packets, gets one, executes what it needs to and sends a response. When the "terminate" packet comes in, that read loop should exit and this should cause the lldb-vscode to exit. So I am questioning if this works around the issue. Does "disconnect" always get sent for both launch and attach? Then "terminate"?

The changes look good for other reasons, but I am wondering if this will fix all of the lldb-vscode binaries that we have sitting around. A quick "sample lldb-vscode" should show where these lldb-vscode binaries are stuck. Can we verify what is going on to make sure this will fix this?

Marking as needs changes due to initialization of the "VSCode::is_attach" not being done in constructor.

lldb/test/API/tools/lldb-vscode/disconnect/main.c

10

This comment seems off? You can call "pause()" to wait for a signal, but that won't work on non unix based systems. Is the comment just left over?

lldb/tools/lldb-vscode/VSCode.h

92

This should be initialized in the VSCode::VSCode constructor? Surely we are initializing the other values right?

[wallace]

Well, the problem that i've seen happens mostly with long running processes like services that just don't die. So this fixes those issues anyway because those processes are not dying when they should. I tried an older version of the IDE and it sends all the time the terminateDebuggee flag.
Anyway, what you mention made me notice something. This is the existing flow without this change

vscode sends a disconnect request. At this point the UI might stop showing the debug session, but it's still running

< lldb-vscode receives it and doesn't terminate the inferior nor terminates itself
< the inferior keeps running and whenever there's output, it's sent to vscode
< if the process terminates, it sends a terminate event

vscode then finishes the debug session

A problem that I see here is that lldb-vscode was still alive. I'll investigate that tomorrow.

Anyway,wWe still need to investigate why some other processes don't die under normal circumstances, but I'll do it in a different patch.

lldb/test/API/tools/lldb-vscode/disconnect/main.c

10

ah yes, it's a left over. I don't want to do something too posix dependant. Good catch

lldb/tools/lldb-vscode/VSCode.h

92

oh thank you, i didn't notice that constructor :)

[labath]

lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py

55

Writing an attach test is tricky because of the yama restrictions on linux. The way that we handle that right now is to have the inferior call lldb_enable_attach() and then create a token file. The test then waits for the token file lldbutil.wait_for_file_on_target and only then attaches. This ensures the attack takes place only when yama is disabled.

68

I'm confused as to why we're not expecting any output here. I was under the impression that we want to let the inferior continue in the "attach" case.

[wallace]

I updated the tests and did some minor changes.

@clayborg, after some testing, I noticed that this doesn't fix the problem of lldb-vscode instances not dying, however it solves the problem
of long-running inferiors not dying. Both are issues anyway.

I'll send another patch fixing the first problem, but at least this solves the second one.

Btw, I mentioned before that even after detaching, lldb-vscode was capturing some output by the inferior, however this is fine.
lldb-vscode captures just some of the inferior's output in its even loop until it gets the eBroadcastBitStopEventThread event,
after that lldb-vscode dies normally. So this diff should be fine.

[clayborg]

lldb/tools/lldb-vscode/lldb-vscode.cpp

799

I am not sure what returning an error to disconnect will do other than hosing the IDE in some way. It really doesn't matter if we succeeded, we should probably just not mess with the return value. Thoughts?

[wallace]

lldb/tools/lldb-vscode/lldb-vscode.cpp

799

This is actually quite useful when analyzing logs, as we could know if any disconnection ended up in an internal failure.
This is safe, as I've tried returning a hardcoded error in this case and the IDE terminated the debug session gracefully anyway. The error field is accepted by the protocol, btw.

[clayborg]

I'm ok with this then if the error being returned for disconnect doesn't affect the IDE. Anyone else should chime in quickly if you have any objections.

[labath]

lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py

68

When I think about this more, I get even more confused. In the attach case, is the inferior stdout even flowing through the vscode connection? I would expect not..

If that's true, then this test and its description is pretty misleading -- there will not be anything in the stdout stream, but not because we disconnected early -- the stdout will _never_ be there.

What exactly are you trying to test here? Maybe it would be more worthwhile to read the stdout from the popen object to ensure that the inferior does continue to print the number correctly?

[wallace]

The tests were weird indeed, I think I had to revisit them after making some changes.
Anyway, I've updated the tests and they do make sense now. They test if the inferior performs some side effect after
it's disconnected.

[wallace]

@labath , does this look better now?

[wallace]

remove unwanted changes

[labath]

Yes, this looks better, though I am mildly worried about the use of timeouts.

There probably isn't anything better we can do for the "launch" case, but we could definitely come up with something better for the "attach" case. I think we can leave the attach code as-is for now, as the symmetry with the launch code is nice, but this will need to be revisited if the test turns out to be flaky.

[MaskRay]

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

lldb/test/API/tools/lldb-vscode/disconnect/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py

+"""
+Test lldb-vscode disconnect request
+"""
+
+
+import unittest2
+import vscode
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbvscode_testcase
+import subprocess
+import time
+import os
+
+
+class TestVSCode_launch(lldbvscode_testcase.VSCodeTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    source = 'main.c'
+
+    def disconnect_and_check_no_output(self):
+        self.vscode.request_disconnect()
+        # verify we didn't get any input after disconnect
+        time.sleep(2)
+        output = self.get_stdout()
+        self.assertTrue(output is None or len(output) == 0)
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_launch(self):
+        """
+            This test launches a process that would output some text to the stdout
+            if we let it run after disconnection.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        self.set_source_breakpoints(self.source, [line_number(self.source, '// breakpoint 1')])
+        self.continue_to_next_stop()
+
+        self.disconnect_and_check_no_output()
+
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_attach(self):
+        """
+            This test attaches to a running process that would output some text to the stdout
+            if we let it run after disconnection.
+        """
+        self.build_and_create_debug_adaptor()
+        program = self.getBuildArtifact("a.out")
+
+        self.process = subprocess.Popen([program, "--attach"],
+                                        universal_newlines=True,
+                                        stdin=subprocess.PIPE,
+                                        stdout=subprocess.PIPE,
+                                        stderr=subprocess.PIPE)
+        self.attach(pid=self.process.pid)
+
+        self.set_source_breakpoints(self.source, [line_number(self.source, '// breakpoint 2')])
+
+        self.process.stdin.write("42 ")
+        self.process.stdin.flush()
+        self.continue_to_next_stop()
+
+        self.disconnect_and_check_no_output()

lldb/test/API/tools/lldb-vscode/disconnect/main.c

+#include <stdio.h>
+
+void for_launch() {
+  int x = 0; // breakpoint 1
+  printf("%d\n", x);
+}
+
+void for_attach() {
+  int x;
+  // We wait for a signal to proceed
+  scanf("%d", &x);
+  printf("%d\n", x); // breakpoint 2
+}
+
+int main(int argc, char **args) {
+  if (argc == 1)
+    for_launch();
+  else
+    for_attach();
+  return 0;
+}

lldb/tools/lldb-vscode/VSCode.h

struct VSCode {
   std::vector<ExceptionBreakpoint> exception_breakpoints;
   std::vector<std::string> init_commands;
   std::vector<std::string> pre_run_commands;
   std::vector<std::string> exit_commands;
   std::vector<std::string> stop_commands;
   lldb::tid_t focus_tid;
   bool sent_terminated_event;
   bool stop_at_entry;
+  bool is_attach;
   // Keep track of the last stop thread index IDs as threads won't go away
   // unless we send a "thread" event to indicate the thread exited.
   llvm::DenseSet<lldb::tid_t> thread_ids;
   VSCode();
   ~VSCode();
   VSCode(const VSCode &rhs) = delete;
   void operator=(const VSCode &rhs) = delete;
   int64_t GetLineForPC(int64_t sourceReference, lldb::addr_t pc) const;

lldb/tools/lldb-vscode/lldb-vscode.cpp

 // "AttachResponse": {
 //   "allOf": [ { "$ref": "#/definitions/Response" }, {
 //     "type": "object",
 //     "description": "Response to 'attach' request. This is just an
 //     acknowledgement, so no body field is required."
 //   }]
 // }
 void request_attach(const llvm::json::Object &request) {
+  g_vsc.is_attach = true;
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
   lldb::SBAttachInfo attach_info;
   auto arguments = request.getObject("arguments");
   const lldb::pid_t pid =
       GetUnsigned(arguments, "pid", LLDB_INVALID_PROCESS_ID);
   if (pid != LLDB_INVALID_PROCESS_ID)

 //                     acknowledgement, so no body field is required."
 //   }]
 // }
 void request_disconnect(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
 
-  bool terminateDebuggee = GetBoolean(arguments, "terminateDebuggee", false);
+  bool defaultTerminateDebuggee = g_vsc.is_attach ? false : true;
+  bool terminateDebuggee =
+      GetBoolean(arguments, "terminateDebuggee", defaultTerminateDebuggee);
   lldb::SBProcess process = g_vsc.target.GetProcess();
   auto state = process.GetState();
 
   switch (state) {
   case lldb::eStateInvalid:
   case lldb::eStateUnloaded:
   case lldb::eStateDetached:
   case lldb::eStateExited:
     break;
   case lldb::eStateConnected:
   case lldb::eStateAttaching:
   case lldb::eStateLaunching:
   case lldb::eStateStepping:
   case lldb::eStateCrashed:
   case lldb::eStateSuspended:
   case lldb::eStateStopped:
   case lldb::eStateRunning:
     g_vsc.debugger.SetAsync(false);
     if (terminateDebuggee)
       process.Kill();
     else
       process.Detach();
     g_vsc.debugger.SetAsync(true);
     break;
   }
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
   SendTerminatedEvent();
   if (g_vsc.event_thread.joinable()) {
     g_vsc.broadcaster.BroadcastEventByType(eBroadcastBitStopEventThread);
     g_vsc.event_thread.join();
   }
 }

 // "LaunchResponse": {
 //   "allOf": [ { "$ref": "#/definitions/Response" }, {
 //     "type": "object",
 //     "description": "Response to 'launch' request. This is just an
 //                     acknowledgement, so no body field is required."
 //   }]
 // }
 void request_launch(const llvm::json::Object &request) {
+  g_vsc.is_attach = false;
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
   g_vsc.exit_commands = GetStrings(arguments, "exitCommands");