This is an archive of the discontinued LLVM Phabricator instance.

[lldb-vscode] Implement RestartRequest
ClosedPublic

Authored by jgorbe on Apr 7 2023, 6:19 PM.

Details

Summary

This is an optional request, but supporting it makes the experience better when
re-launching a big binary that takes significant time to parse: instead of
tearing down and re-create the whole session we just need to kill the current
process and launch a new one.

Some non-obvious comments that might help review this change:

  • After killing the process, we get an "exited" event for it. Because the process no longer exists some interesting things can occur that manifest as flaky failures if not dealt with:
    • EventIsProcessEvent relies on SBEvent::GetBroadcasterClass, which can crash if the broadcaster is no longer there: the event only holds a weak_ptr to its broadcaster, and GetBroadcasterClass uses it without checking.

      Other EventIs* functions look at the flavor of the EventData, so I have modified EventIsProcessEvent to do that.
    • We keep the PID of the old process so we can detect its "exited" event and not terminate the whole session. But sometimes the SBProcess we get from the event won't have a PID, for some reason.
  • I have factored out the code to launch a process out to a new LaunchProcess function, so it can be used from both request_launch and request_restart.
  • The restart_runInTerminal test has the same problem with debug builds as the original runInTerminal test: when attaching to the launcher instance of lldb-vscode it takes a long time to parse its debug info. I have used the same workaround to disable that particular test for debug builds.

Diff Detail

Event Timeline

jgorbe created this revision.Apr 7 2023, 6:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2023, 6:19 PM
jgorbe requested review of this revision.Apr 7 2023, 6:19 PM
rupprecht added inline comments.Apr 10 2023, 1:21 PM
lldb/tools/lldb-vscode/VSCode.h
147

std::optional<llvm::json::Object>? And then we can raise an error if the client calls restart and this is std::nullopt.

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

Should we also set last_launch_or_attach_request here?

If the user runs:

  1. Launch
  2. Attach
  3. Restart

I would expect that should fail because of the reasons restarting an attach will fail. But if we don't set last_launch_or_attach_request, it will pass, because we set it in (1).

jgorbe updated this revision to Diff 512305.Apr 10 2023, 6:46 PM

Addressed review comments.

  • Changed last_launch_or_attach_request to be a std::optional<llvm::json::Object> so we can check if it has been set. Also check for nullopt in request_restart.
  • Made request_attach also set last_launch_or_attach_request.
jgorbe marked 2 inline comments as done.Apr 10 2023, 6:46 PM
jgorbe added inline comments.
lldb/tools/lldb-vscode/VSCode.h
147

Done, thanks!

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

Yup, I missed that one, thanks for catching it! :)

jgorbe marked 2 inline comments as done.Apr 10 2023, 6:47 PM

Looks ok-ish, though I don't feel entirely comfortable approving lldb-vscode changes. @wallace, do you have any thoughts on this?

lldb/tools/lldb-vscode/VSCode.h
152

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).

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

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...

wallace added inline comments.Apr 28 2023, 9:01 AM
lldb/source/API/SBProcess.cpp
782–783

+1

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

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?

jgorbe added inline comments.May 1 2023, 6:11 PM
lldb/tools/lldb-vscode/VSCode.h
152

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).

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

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 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.

1983

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?

wallace accepted this revision.May 1 2023, 7:12 PM

lgtm!

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

i think it's better to reuse the same arguments that the user passed using the vscode interface. Modifying them in the command line might seems counterintuitive.

This revision is now accepted and ready to land.May 1 2023, 7:12 PM
This revision was automatically updated to reflect the committed changes.