[Lldb-commits] [PATCH] D79726: Add terminateCommands to lldb-vscode protocol

Tue May 12 11:49:34 PDT 2020

aadsm marked an inline comment as done.
aadsm added inline comments.

================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py:250-251
         def cleanup():
-            self.vscode.request_disconnect(terminateDebuggee=True)
+            if disconnect:
+                self.vscode.request_disconnect(terminateDebuggee=True)
             self.vscode.terminate()
----------------
labath wrote:
> aadsm wrote:
> > labath wrote:
> > > What's the purpose of this argument? To ensure a clean shutdown? Would it be possible to make the function smart enough to detect the right thing to do when cleaning up?
> > it indicates if the process should be killed or not when lldb-vscode disconnects from it.
> I meant the `disconnect` argument to the `attach` function in the test suite , not the `terminateDebuggee` argument of the `disconnect` command to lldb-vscode.
> 
> My point is that, given the way the cleanup functions work, you cannot assume that you're going to be invoked in any particular moment. For example they can called if an assertion fails, to ensure any external resources/processes are terminated. Since you don't know which assertion will fail, it's best if you don't assume anything about the the state of the inferior and just do your best to clean up. I'm not sure what "doing your best" would mean here. For example it may mean sending a "terminate" request and ignoring errors. Or maybe a better solution is possible...
oh yeah of course, the argument I added 😅. Oh, you know what, I misread when the cleanup function was going to be called. I don't need this at all, will remove it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79726