Infinite Loop in DAP when sending request before launch.

[da-viper]

I noticed this while implementing the supportsDataBreakpointBytes

This adds an option to the break-point panel in vscode, to set an address range. It is also persistent between debug sessions.

lldb-dap goes into an infinite loop because, The current implementation creates pending requests places request based on the below filter,

llvm-project/lldb/tools/lldb-dap/DAP.cpp

Lines 927 to 933 in efae492

	disconnecting = true;
	if (!configuration_done)
	add_to_pending_queue =
	command != "initialize" && command != "configurationDone" &&
	command != "disconnect" && !command.ends_with("Breakpoints");
	}

Since VSCode is waiting for a response for the databreakpointInfo request it will not send the configurationdone request.

Since lldb-dap is waiting for the configurationDone request it will not start the launch or attach request, and it is stuck in an infinite loop.

Normally adding the databreakpointinfo to the filter would suffice, but it will break existing binary when there is a new capability.

Example scenario.

• User debugs a program with debugger A and set's a dataBreakpointInfo
• User end debugger A session.
• User starts debugging with lldb-dap.
• lldb-dap enters an infinite loop because it cannot respond to the request.

Another way to mediate the issue is to handle it the way it is done in lldb

(lldb) b main
Breakpoint 1: no locations (pending).
Breakpoint set in dummy target, will get copied into future targets.

During the launch request and attach request we create the target and after the configurationDone request we then launch the debuggee. This is something similar to

# During launch request
(lldb) target create a.out
Current executable set to '/home/da-viper/Dev/projects/test_lldb/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 18 at main.cc:21:7, address = 0x00000000004005f2

# After configuration done request
(lldb) run 
Process 136009 launched: '/home/da-viper/Dev/projects/test_lldb/a.out' (x86_64)

This also deals with the issue of unverified breakpoints as breakpoints can be set with a valid target.

Relevant stuck lldb-dap with the logs on the side.

Pinging for feedback in case I am missing anything before implementing the fix.
@JDevlieghere @ashgti @ely @kazutakahirata

[llvmbot]

@llvm/issue-subscribers-lldb-dap

Author: Ebuka Ezike (da-viper)

I noticed this while implementing the supportsDataBreakpointBytes

This adds an option to the break-point panel in vscode, to set an address range. It is also persistent between debug sessions.

lldb-dap goes into an infinite loop because, The current implementation creates pending requests places request based on the below filter,

llvm-project/lldb/tools/lldb-dap/DAP.cpp

Lines 927 to 933 in efae492

	disconnecting = true;
	if (!configuration_done)
	add_to_pending_queue =
	command != "initialize" && command != "configurationDone" &&
	command != "disconnect" && !command.ends_with("Breakpoints");
	}

Since VSCode is waiting for a response for the databreakpointInfo request it will not send the configurationdone request.

Since lldb-dap is waiting for the configurationDone request it will not start the launch or attach request, and it is stuck in an infinite loop.

Normally adding the databreakpointinfo to the filter would suffice, but it will break existing binary when there is a new capability.

Example scenario.

• User debugs a program with debugger A and set's a dataBreakpointInfo
• User end debugger A session.
• User starts debugging with lldb-dap.
• lldb-dap enters an infinite loop because it cannot respond to the request.

Another way to mediate the issue is to handle it the way it is done in lldb

(lldb) b main
Breakpoint 1: no locations (pending).
Breakpoint set in dummy target, will get copied into future targets.

During the launch request and attach request we create the target and after the configurationDone request we then launch the debuggee. This is something similar to

# During launch request
(lldb) target create a.out
Current executable set to '/home/da-viper/Dev/projects/test_lldb/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 18 at main.cc:21:7, address = 0x00000000004005f2

# After configuration done request
(lldb) run 
Process 136009 launched: '/home/da-viper/Dev/projects/test_lldb/a.out' (x86_64)

This also deals with the issue of unverified breakpoints as breakpoints can be set with a valid target.

Relevant stuck lldb-dap with the logs on the side.

Pinging for feedback in case I am missing anything before implementing the fix.
@JDevlieghere @ashgti @ely @kazutakahirata

[ashgti]

I think your describing how we handled the startup sequence before #138219.

I'm a bit surprised that the addresses would be reused between sessions though because the memory addresses seem pretty specific to the existing process.

Should we set "canPersist": false? Is that what your doing in your implementation?

[da-viper]

I think your describing how we handled the startup sequence before #138219.

The difference is that, we will not launch immediately but after configuration done (previously we launched/ attached immediately).

I'm a bit surprised that the addresses would be reused between sessions though because the memory addresses seem pretty specific to the existing process.

• It can still be pointing to the same variable if it is on the stack and ASLR is disabled and it is a small program (quite a lot of conditions).
• If you are debugging in embedded setting and the address is a Memory-Mapped Address.

Should we set "canPersist": false? Is that what your doing in your implementation?

Currently I don't set the canPersist, I put the implementation on pause since this seems to have more priority.
The loop can also occur if another extension create the breakpoint before we launch lldb.

[JDevlieghere]

Is this another instance where Visual Studio Code isn't compliant with the DAP spec? It says that between the initialized event and the configurationDone, we should only get "configuration requests". The spec doesn't actually define "configuration requests", but I would argue that DataBreakpointInfo isn't one of them.

Regardless of whether it's compliant, I agree that we need a way to support this. Seems like we have two options:

1. Try to handle the request to the best of our ability.
2. Respond with a failure when we get this request before we can handle it.

No matter which option we pick, it needs to bypass the deferred request filter, or we need to stop deferring requests.

Normally adding the databreakpointinfo to the filter would suffice, but it will break existing binary when there is a new capability.

I'm not sure what you mean by this. If the debug adapter starts supporting and advertising a new capability it would have to change anyway.

Another way to mediate the issue is to handle it the way it is done in lldb

What you describe (setting breakpoints in the dummy target) is how things already work today when you set breakpoints before the launch/attach request.

[ashgti]

Is this another instance where Visual Studio Code isn't compliant with the DAP spec? It says that between the initialized event and the configurationDone, we should only get "configuration requests". The spec doesn't actually define "configuration requests", but I would argue that DataBreakpointInfo isn't one of them.

VSCode is checking if the session is readyForBreakpoints before calling dataBreakpointInfo, which is true after we send the initialized event (https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/debug/browser/debugSession.ts#L577 and https://github.com/microsoft/vscode/blob/e7894219a0b6b9f1cb7e2db79b25cb6416c95ef1/src/vs/workbench/contrib/debug/browser/rawDebugSession.ts#L112). So this would probably qualify as one of the "client sends other future configuration requests" items from the spec for the initialized event.

[JDevlieghere]

Alright, if that's the interpretation, then I don't see another option but to do what you're describing in #140299. Until I realized that the client sends the launch/attach and configuration in parallel, I couldn't think of a solution that would support all scenarios and be compliant with the spec.