[Lldb-commits] [PATCH] D147831: [lldb-vscode] Implement RestartRequest

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe added inline comments.


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:152
   bool is_attach;
+  // The process event thread normally responds to process exited events by
+  // shutting down the entire adapter. When we're restarting, we keep the id of
----------------
labath wrote:
> So, it's not possible to restart a process if it has already terminated? I.e., if the debugged process unexpectedly exits, the user will have to restart lldb-vscode from scratch (so he can e.g., set a breakpoint in before the exit).
I don't know if it's //possible// (as in, if the protocol allows it), but it's what lldb-vscode does.

It also matches the behavior I remember when I used Visual Studio back in the day: when the debugged process exits, the debugging session ends and the UI switches back to "editing" mode automatically. If you need to re-run, you hit F5 and run it again. It's usually not a big deal because the IDE keeps the list of breakpoints and will transparently start lldb-vscode and set everything up again when you click "Run". It's also what VS Code does when a debug adapter doesn't support the RestartRequest, it just kills the adapter and restarts everything transparently (which is how restart works on lldb-vscode so far).


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1976
+    if (state != lldb::eStateConnected) {
+      process.Kill();
+    }
----------------
wallace wrote:
> Kill actually detaches if lldb originally attached to the debuggee. The actual implementation says
> 
> ```
>   /// Kills the process and shuts down all threads that were spawned to track
>   /// and monitor the process.
>   ///
>   /// This function is not meant to be overridden by Process subclasses.
>   ///
>   /// \param[in] force_kill
>   ///     Whether lldb should force a kill (instead of a detach) from
>   ///     the inferior process.  Normally if lldb launched a binary and
>   ///     Destory is called, lldb kills it.  If lldb attached to a
>   ///     running process and Destory is called, lldb detaches.  If
>   ///     this behavior needs to be over-ridden, this is the bool that
>   ///     can be used.
>   ///
>   /// \return
>   ///     Returns an error object.
>   Status Destroy(bool force_kill);
> ```
> You could have the restart command reattach to the process instead of failing if the user originally attached. 
> 
> What do you think of this?
I think it's something you *could* do, but it's not restarting. As I mention in a code comment above, I don't think "restart" is even well defined in the attach case.

As a user I would be really confused if I clicked on restart and the debugger deattached from the process, then immediately reattached, but nothing restarted. I can't think of a case where this would be useful.

Looking for some references, I started Visual Studio (not Code) and it simply grays out the "restart" button when attaching. Visual Studio Code, however, allows you to restart. I found [this issue](https://github.com/microsoft/debug-adapter-protocol/issues/73) where someone asked for clarification on the meaning of Restart in the protocol and got this explanation:

> "restart sequence" means:
> terminate current debug session via the disconnect request and immediately start a new session via the launch or attach request
> 
> For the "launch" case "restart" can be used to pickup source changes that are not automatically picked up while the program is running.
> 
> For the "attach" case "restart" has no direct effect on the debug target because the debug target continues running. But it still might have an effect on debugging because the debug adapter is restarted and this could pick up changes to files that the debug adapter uses, e.g. source maps.

But I'm not very convinced by this because we're implementing RestartRequest, which means precisely that the adapter won't be restarted.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1983
+  g_vsc.debugger.SetAsync(true);
+  LaunchProcess(*g_vsc.last_launch_or_attach_request);
+
----------------
labath wrote:
> I have no idea if that's a good idea or not, but I'm wondering if, instead of from the last attach request, we should be taking the arguments from lldb. So that if the user say changes the `target.run-args` setting, then the new restart request will take that into account...
I think the intended way to allow the user to modify the configuration is to use the optional `arguments` field in `RestartRequest`, which the client can use to send the latest version of the config.

https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Restart

> ```
> interface RestartRequest extends Request {
>   command: 'restart';
> 
>   arguments?: RestartArguments;
> }
> ```
> Arguments for restart request.
> 
> ```
> interface RestartArguments {
>   /**
>    * The latest version of the `launch` or `attach` configuration.
>    */
>   arguments?: LaunchRequestArguments | AttachRequestArguments;
> }
> ```

We can check if the restart request has `arguments` and only used the saved config if that's not the case. What do you think?

@wallace any opinions here? Is this kind of lldb state manipulation via console commands something that is expected to work correctly in lldb-vscode?


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

https://reviews.llvm.org/D147831