labath accepted this revision. labath added a comment. This revision is now accepted and ready to land.
In D79726#2031817 <https://reviews.llvm.org/D79726#2031817>, @aadsm wrote: > > I gotta say I don't understand the difference between "exit" and > > "terminate" commands, as both seem to happen pretty at once. Is the > > difference that "exit" commands are run only when the inferior exits freely > > on its own, and not when we forcefully kill it? And that "terminate" cmds > > are run in both cases? > > Yeah, just like said, the exit event (and commands) only happen when the > program normally exits (with an exit code). However, I want to run commands > when the debugging session is over, which might not necessarily mean the > debuggee has ended (forcefully or not), only that we stopped debugging it. My > main goal is to collect stats of the debugging session that has just ended. Aha, intereseting. So the "exit" commands would not run if we detach from the process, but the "terminate" commands still would. That makes sense to me. ================ 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() ---------------- aadsm wrote: > aadsm wrote: > > 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. > sorry about this, I remember now (I've been working on something else not > related to this when I replied). > > I did it this way so I can explicitly say that I don't want the test to > automatically disconnect when it's teared down. I need to disconnect manually > so I can test the terminateCommands, and and 2 disconnects won't work well > because we'll be waiting for that answer that will never arrive, since it has > already been terminated. > > Another way to do this, that I thought at the time, was to track if the test > is connected or not and only disconnect in that situation. I opted to not do > that in the end because I wanted this behaviour to be intentional in the > test, to prevent something from disconnecting (unintentionally, like a bug) > and then the teardown will skip it and that error would never be surfaced. > This might be a bit overzealous though... > > what do you think? > > > Yeah, I think it would be better if this tracked when it is necessary to terminate the child automatically, or just sent the terminate request blindly (ignoring errors). The main purpose of the clean up commands is to "clean up" after the test, generating an exception here can just prevent other cleanups from happening. That said, I don't think this is too important. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79726/new/ https://reviews.llvm.org/D79726 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
