eclipse-cdt-cloud / cdt-gdb-adapter

CDT GDB Debug Adapter
Eclipse Public License 2.0
28 stars 40 forks source link

Allow pre-filling launch/attach on command line to adapter #228

Closed jonahgraham closed 1 year ago

jonahgraham commented 1 year ago

--config=INITIALCONFIG

Start the adapter using the given configuration as a starting point for the args in launch or attach request.

For example, the default GDB can be set like this:

    node debugTargetAdapter.js --config='{"gdb":"arm-none-eabi-gdb"}'

The config can be passed on the command line as JSON, or a response file can be used by starting the argument with @. The rest of the argument will be interpreted as a file name to read. For example, to start the adapter defaulting to a process ID to attach to, create a file containing the JSON and reference it like this:

    cat >config.json <<END
    {
      "processId": 1234
    }
    END
    node debugAdapter.js --config=@config.json

--config-frozen=FROZENCONFIG

Similar to --config, the --config-frozen sets the provided configuration fields in the args to the launch or attach request to the given values, not allowing the user to override them. Specifying which type of request is allowed (launch or attach) can be specified with the request field.

For example, the adapter can be configured for program to be frozen to a specific value. This may be useful for starting adapters in a container and exposing the server port.

    node debugAdapter.js --server=23221 --config-frozen='{"program":"/path/to/my.elf"}'

Fixes #227

thegecko commented 1 year ago

Thanks @jonahgraham, this is exactly what I was thinking.

A couple of observations considering the "server" owner should potentially own the setup configuration.

e.g. considering:

            args = {
                ...GDBDebugSession.commandLineLaunchAttachArguments,
                ...args,
            };

A situation could exist where a remote server specifies the binary to be debugged, but the remote client can override this and point at arbitrary binaries.

For now I don't belive these points should block merging, though!

jonahgraham commented 1 year ago

For now I don't belive these points should block merging, though!

OK. I'll test this draft works and work towards merging.

Should the server somehow dictate whether the session is a launch or attach? When configuration is specified in the server, could it be a security concern to allow the client to override the data?

I'll consider how to address this as I test.

jonahgraham commented 1 year ago

Updated description to include details on --config-frozen that I added to address @thegecko's concerns raised.

jonahgraham commented 1 year ago

@thegecko this is ready for review (docs + tests done)

jonahgraham commented 1 year ago

The change (hopefully just the tests) don't work on Windows. I'll have to spin up there to figure out what went wrong, but my guess is it has to do with how we are passing the extra args in the debug adapter launch here:

https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/2ceef5b03b0cfd8b70cb6fcaf386e19ff17670a9/src/integration-tests/utils.ts#L193-L203

That code relies on shell=true so that all the arguments can be passed as one parameter, but with all the quoting my guess is on Windows the shell handling causes the arguments to be split in the wrong place. I have some WIP on allowing this in cdt-gdb-adapter here, but the better solution would be to fix it upstream, probably like this, but that isn't tested.

jonahgraham commented 1 year ago

The change (hopefully just the tests) don't work on Windows.

Windows issues have been fixed, all CI green now.

jonahgraham commented 1 year ago

@thegecko As discussed I have refactored launch/attach request to have a true common code for both of them in 594348825e0a76185e0e0469e91bb39a3bf21a17

The final commit 55510156bd2231a9f30bdc408a2387505c5f586d in this series is the implementation of the feature request. The preceding commits are all the cleanup/refactoring that I did to make it easier to read and review 55510156bd2231a9f30bdc408a2387505c5f586d