Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jan 26, 2023

Conversation

jonahgraham
Copy link
Contributor

@jonahgraham jonahgraham commented Jan 18, 2023

--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 [email protected]

--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
Copy link
Contributor

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

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

  • 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?

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@jonahgraham jonahgraham marked this pull request as ready for review January 23, 2023 18:34
@jonahgraham
Copy link
Contributor Author

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

@jonahgraham
Copy link
Contributor Author

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:

export async function standardBeforeEach(
adapter?: string,
extraArgs?: string
): Promise<CdtDebugClient> {
let args = getAdapterAndArgs(adapter);
if (extraArgs) {
args = `${args} ${extraArgs}`;
}
const dc: CdtDebugClient = new CdtDebugClient('node', args, 'cppdbg', {
shell: true,
});

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.

Copy link
Contributor

@thegecko thegecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small request to remove a check, but apart from that this works well for me. I'll share an example of it in use shortly

src/GDBDebugSession.ts Show resolved Hide resolved
@jonahgraham
Copy link
Contributor Author

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

Windows issues have been fixed, all CI green now.

The majority of the code in attach/launch request is the same
with just small differences based on whether to execute the
target program or not.

This is a code cleanup that makes it easier to implement eclipse-cdt-cloud#227
Prior to this change the error message said "Error: arg is not iterable"
it now says "The program must be specified in the launch request arguments"
Most of the timeouts for tests are the mocha timeouts, but DebugClient
has its own timeout values. This change synchronizes those values.

This increased timeout is specifically needed for the attachRemote
test which takes more than 5 seconds to do the initial connection
on the GitHub actions build regularly.

This is a follow up to commit 3708cea
part of eclipse-cdt-cloud#222
Remove shell=true when spawning adapter in tests

The shell=true was a workaround so that arguments could be
passed to node. But it leads to platform specific handling.
Therefore remove the shell=true which also means the workarounds
that would have been needed to quote JSON on the command line
properly are not needed.

Needed to test eclipse-cdt-cloud#227
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:

```sh
    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:

```sh
    cat >config.json <<END
    {
      "processId": 1234
    }
    END
    node debugAdapter.js [email protected]

```

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.

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

Fixes eclipse-cdt-cloud#227
@jonahgraham
Copy link
Contributor Author

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

The final commit 5551015 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 5551015

Copy link
Contributor

@thegecko thegecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jonahgraham. I tested passing an attach config with a frozen launch server config and all works as expected.

I'd be keen to see a new release when this gets merged :)

@jonahgraham jonahgraham merged commit 7e2014f into eclipse-cdt-cloud:main Jan 26, 2023
@jonahgraham jonahgraham deleted the draft_for_227 branch January 26, 2023 14:47
jonahgraham added a commit to jonahgraham/cdt-gdb-adapter that referenced this pull request Apr 13, 2023
There is no strict need to have a program passed to GDB when
attaching (remote or local). Either the program can be downloaded
from the remote or otherwise read by GDB, or if not possible
then GDB can do symbol-free debugging.

Previously in eclipse-cdt-cloud#228 some work was done to better error when
program was not specified. This change loosens the restrictions
slightly. If the program is not specified, it is still a warning
in VSCode because of
[package.json](https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/blob/22a9f1048f9030689ed1f5f2b3e5d5e48df9047d/package.json#L205-L207)

See this comment for how it is presented to a user:
eclipse-cdt-cloud#261 (comment)

Part of eclipse-cdt-cloud#261
jonahgraham added a commit to jonahgraham/cdt-gdb-adapter that referenced this pull request Apr 21, 2023
There is no strict need to have a program passed to GDB when
attaching (remote or local). Either the program can be downloaded
from the remote or otherwise read by GDB, or if not possible
then GDB can do symbol-free debugging.

Previously in eclipse-cdt-cloud#228 some work was done to better error when
program was not specified. This change loosens the restrictions
slightly. If the program is not specified, it is still a warning
in VSCode because of
[package.json](https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/blob/22a9f1048f9030689ed1f5f2b3e5d5e48df9047d/package.json#L205-L207)

See this comment for how it is presented to a user:
eclipse-cdt-cloud#261 (comment)

Part of eclipse-cdt-cloud#261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support configuration when in Server Mode
2 participants