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

Run gdb commands before connection #97 #99

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Gaswigue
Copy link

@Gaswigue Gaswigue commented Mar 8, 2017

No description provided.

@WebFreak001
Copy link
Owner

It would be cool if environment-directory and target-select could also be inside the autorunBeforeCmds array. In line 232 where you only have sendCommand right now you would need to add .replace(/\$escaped_cwd/g, escape(cwd)).replace(/\$cwd/g, cwd).replace(/target/g, target) after the command string. This would make it much more configurable and it is really easily done like that. Then you can add "environment-directory \"$escaped_cwd\"" and "target-select remote $target" to the autorunBeforeCmds array

@Gaswigue
Copy link
Author

Gaswigue commented Mar 8, 2017

It cross my mind to do as you say.
But as i am not very good at typescript/JS, I though there was a much better way to do so i didnt do it.
Thanks to your help. I'll check on that.

@WebFreak001
Copy link
Owner

If you need any more help or suggestions, just push the code and I can use the github review feature. I think it's important to add this to SSH too, but you don't need to do that, that can be done in a separate PR or commit by me if you don't want to do it

@Gaswigue
Copy link
Author

Gaswigue commented Mar 8, 2017

Ssh is not needed for me but I will code the improvement for this use case also. I have some other ideas ;)

this.miDebugger = new MI2(args.gdbpath || "gdb", ["-q", "--interpreter=mi2"], args.debugger_args);
this.initDebugger();

private initiliaseDefaultValueArgs(args: CommonRequestArguments, attach: boolean) {
Copy link
Author

@Gaswigue Gaswigue Mar 9, 2017

Choose a reason for hiding this comment

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

private initiliaseDefaultValueArgs(args: CommonRequestArguments, attach: boolean) {
New function to initialize unseted variable.
This avoid the launchRequest/attachRequest copy/past

Copy link
Owner

Choose a reason for hiding this comment

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

The other parts of the program spell it initialize instead of initialise, can you keep it consistent and change it to the american spelling?

s => {return escape(s);
});

args.autorunBefore = args.autorunBefore.map(
Copy link
Author

Choose a reason for hiding this comment

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

args.autorun = args.autorun.map(s => {return escape(s);});
Resolve \ escape in $(workspaceroot) if we use it to load binary from gdb (command load)

Copy link
Owner

Choose a reason for hiding this comment

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

don't use escape for this, it will put a \ before all other \ and " characters, this will manipulate user input and it will break all commands using " or \. I'm not quite sure what you mean with the workspaceroot.

Copy link
Author

@Gaswigue Gaswigue Mar 9, 2017

Choose a reason for hiding this comment

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

I have a set of commands to send to gdb after connection
"load ${workspaceRoot}\\bin\\binary.x"
if i don't escape the path is wrong it remove all the \
the final command is
"load c:somedirAsomedirB\bin\binary.x" and not c:\somedirA\somedirB\bin\binary.x

Copy link
Author

Choose a reason for hiding this comment

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

I try to do
load ".\bin\binary.x" and it work. no need to use workspaceRoot. I can delete if needed this part.

Copy link
Owner

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

Looks like a good start, there were different commands depending if they were launch or attach, I need to check what this changes later, the PR shouldn't break any existing debugging configurations (so exactly the same commands should be sent if users don't provide custom commands). For example in lldb it is like that

"description": "mago commands to be run before connection",
"default": [
"gdb-set target-async on",
"environment-directory \"$cwd\"",
Copy link
Owner

Choose a reason for hiding this comment

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

Just having $cwd without the quotes and instead adding the quotes in the replace function would be a better idea I think because it makes the default more clean and it's more obvious to the user because there are no random quotes around it.

let args = [];
if (executable && !nativePath.isAbsolute(executable))
executable = nativePath.join(cwd, executable);
if (executable)
args = args.concat([executable], this.preargs);
else
args = this.preargs;
args = this.preargs;
Copy link
Owner

Choose a reason for hiding this comment

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

there is a leading whitespace here

protected launchRequest(response: DebugProtocol.LaunchResponse, args: LaunchRequestArguments): void {
this.miDebugger = new MI2_Mago(args.magomipath || "mago-mi", ["-q"], args.debugger_args);
this.initDebugger();
private initiliaseDefaultValueArgs(args: CommonRequestArguments, attach: boolean) {
Copy link
Owner

Choose a reason for hiding this comment

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

Format your changed files with the vscode typescript formatter (alt-shift-f or ctrl-shift-i depending on your platform) please

autorunBeforeCmds.forEach(command => {
cmds.push(this.sendCommand(command));
});
cmds.push(this.sendCommand("file-exec-and-symbols \"" + escape(executable) + "\""));
Copy link
Owner

Choose a reason for hiding this comment

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

This is already in the autorunBeforeCmds

@GitMensch
Copy link
Collaborator

What is the state of this PR? Is it a draft? Is it abandoned?

@GitMensch
Copy link
Collaborator

GitMensch commented Mar 4, 2022

It's been a while...
I'd like to take that up on current master (ideally after #323 and #316 were merged as those change the initialization):

Opinions?

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.

4 participants