microsoft / vscode-debugadapter-node

Debug adapter protocol and implementation for VS Code.
Other
273 stars 79 forks source link

Debug Protocol: Support checksums for source files #63

Closed andrewcrawley closed 8 years ago

andrewcrawley commented 8 years ago

We'd like to be able to provide checksums for files that the debug adapter could use to verify that the source files shown in the UI match the source files used to build the debuggee.

Checksum algorithms supported in VS include:

MIEngine supports several additional checksum types intended to be resilient against changes in line endings:

Other data that would allow file mismatches to be detected could be returned in the same way, such as a timestamp for the source file or binary.

export interface Capabilities {
    // ...
    /** Checksum algorithms supported by this debug adapter */
    supportedChecksumAlgorithms?: string[];
    // ...
}

export interface Source {
    // ...
    /** Checksums associated with this file */
    checksums?: Checksum[];
    // ...
}

export interface Checksum {
    /** Name of the checksum algorithm */
    algorithm: string;
    /** Value of the checksum */
    checksum: string;
}

@jacdavis @gregg-miskelly @richardstanton @tzwlai

gregg-miskelly commented 8 years ago

One minor tweak that I didn't think of before: instead of having the algorithm declared as a string, we should probably use an enum so that it is obvious what the names should be. Otherwise LGTM.

andrewcrawley commented 8 years ago

We'll verify this proposal against our scenarios and send a PR.

weinand commented 8 years ago

@andrewcrawley could you please describe when and how these protocol additions would be used?

andrewcrawley commented 8 years ago

The basic idea is that when we interact with a source file (opening it, hitting / setting a breakpoint in it, etc), we need to be able to verify that it's the same source file that was used when the module was built. The debug adapter would provide the checksum of the file that was built into the module, and the UI would compare that against the checksum of the local file, and either produce a warning or prompt the user to locate the correct copy if there's a mismatch.

weinand commented 8 years ago

Thanks for providing this additional detail. My problem was understanding in which direction the checksum is flowing (because the Source type is used in both directions).

In theory you do not need this protocol addition because you could already implement this check in your debug adapter in the following way: Since the debug adapter has access to the source in the file system, it can compute the checksum of a source file and compare it with the checksum of the compiled version. If they do not match, this problem could be returned in the 'origin' attribute of the Source object (and VS Code will show this in the editor header). The nice property of this approach is that the checksum algorithm doesn't leak into VS Code (and doesn't introduce another dependency between front- and backend). We are using this approach in the node debugger already and in this case the algorithm is very specific to node.js and doesn't make sense anywhere else so we would not want to have this algorithm in VS Code itself.

Do you think you could use this approach (or an improved variant of this approach) in VS as well?

Capabilities.supportedChecksumAlgorithms: why do you need the set of algorithms upfront in the UI? I would have expected that the frontend passes the set of supported algorithms in the InitializeRequestArguments to the adapter so that the adapter only computes checksums with the algorithms supported by the frontend.

andrewcrawley commented 8 years ago

Sorry, I didn't mean to imply that this was strictly one-way. Checksums would be bi-directional - one scenario we want is to send a SetBreakpointRequest with checksums set on the Source objects and have the debug adapter's response indicate that the breakpoints failed to bind if the checksums don't match. In this case, the UI would need to know what types of checksums the debug adapter expects, hence the list of supported algorithms on the Capabilities.

Also, I'm not sure we can count on the debug adapter having access to the sources. The debug adapter has access to the checksums of the files that were used to compile the module, since they're in the PDBs.

gregg-miskelly commented 8 years ago

RE: why add this to the protocol instead of just having the debug adapter read the source --

UI -> adapter: When passing the checksum to the debug adapter (ex; breakpoint binding) the source-on-disk may not exactly match what the user has open. By having the editor calculate the value we can avoid this problem. I don't know that we should require the editor to calculate this -- our debug adapter today reads the source file as that is the best we can do today. But when the IDE is VS we want to be able to pass this down to the debug adapter so that we can have fidelity with our experience today.

Adapter -> UI: In VS (and hopefully someday in other IDEs) we want to allow the UI to participate in source file locating rather than making this entirely the adapter's job. In VS, source file location is actually almost all the UI's job. But for this to work, we need the UI aware of checksums so that it can determine if the file it found with the right name is really the right file. This enables, for example, if the user grabbed binaries built by their build server on f:\builds, but they have a source file open in c:\myenlistment, then the UI can magically figure out where sources are. And in situations where it can't do this entirely magically, it can use a File->Open dialog to just let the user pick a file. All without extra configuration.

I am not sure if we need either of these in VS Code, but we absolutely need to be able to support them in VS.

remss commented 6 years ago

What is current behavior of the supportedChecksumAlgorithms DebugAdapter capability ? How do we get the dirty source checksum on setBeakpoints request to not apply them in the debugee and set verified=false ? (because source changed and does not match any more)