add support for envFile #358
Conversation
| if (!(envFile || procEnv)) | ||
| return; | ||
|
|
||
| // FIXME: if actuall debugged process is Win32, which we only know _after_ connect, |
There was a problem hiding this comment.
Do we not really not know what type of process that will be debugged? The environment support is only applicable to the debugger running on the same machine as the debug adapter (not supported over SSH), so it should be able to determine the system it's running on via process.platform. Not sure I understand this issue.
There was a problem hiding this comment.
No, we don't know. I'm regularly using gdb to attach to gdbserver on a different machine myself. Works fine as long as the gdb has support for the target's architecture.
There was a problem hiding this comment.
But these environment variables are only being applied to the process where gdb is being run, not the process where gdbserver is run. I don't think I understand what the expectation of the environment variables would be in a remote scenario like that. Are they expected to be visible to the application being debugged?
There was a problem hiding this comment.
Are they expected to be visible to the application being debugged?
likely yes, as long as we use set environment (or its mi equivalent) this will work both for GDB and (at least on GNU/Linux environments) gdbserver (since GDB 8.1); for running processes wed need to call into putenv()`, but I think it is fine to say that this is only supported for new inferiors.
| @@ -886,3 +889,32 @@ export class MI2 extends EventEmitter implements IBackend { | |||
| protected stream; | |||
| protected sshConn; | |||
| } | |||
| function readEnvFile(filename: string): { [key: string]: string } { | |||
There was a problem hiding this comment.
I would probably recommend taking these functions, along with the functionality in the MI2 constructor above and moving it to its own class (e.g., "Environment"). This would encapsulate this functionality and make it easy to unit test.
There was a problem hiding this comment.
Yes, that's a general issue with the existing code - the environment variables are expected to be found in the inferior (this "just happens" when GDB starts it locally). If we cannot make this happen all environment settings should be dropped from all remote scenarios.
I think this should be changed in general to use; and I even forgot if there's a better way than call setenv (name, value, 1) [which likely only work for C/C++ code, not everything GDB supports].
There was a problem hiding this comment.
There does appear to be commands in both gdb and lldb for setting environment variables, so using that approach, rather than attempting to set them at the process level might be the way to go. The environment variable setting commands could just be issued during the MI2::initCommands, similarly to how the path mappings are handled (i.e., extra commands). The GDB documentation even says these environment variable settings are sent to gdbserver, so should support remote debugging as well (see here). This should also work with SSH connections too, since the commands are issued to the debugger itself.
- gdb Add/Remove Environment Variable:
(gdb) set environment FOO=BAR
(gdb) unset environment FOO- lldb Add/Remove Environment Variable:
(lldb) settings set target.env-vars FOO=BAR
(lldb) settings remove target.env-vars FOO
|
I'm not (possibly anymore) familiar with the test setup - do we have a dev doc how to add tests and run them (outside of CI)? |
The HACKING document does provide a brief overview in the testing section. The Unit tests use Mocha and I recommended the Mocha Test Explorer extension as an easy way to run the unit tests interactively. |
fix #258
so far untested; it is likely possible to also add unit tests; so work in progress (@ any one, feel free to take over)