-
Notifications
You must be signed in to change notification settings - Fork 598
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
Support interposing a (very basic) gdb script that sets debug file paths in the sources
command.
#3714
Conversation
We should do something about that, but not in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think we're going to need tests for this and (sooner rather than later) other SourcesCommand
functionality.
index = strcspn(buf, "\n"); | ||
buf[index] = 0; | ||
|
||
char* token = strtok(buf, delimiter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to regret not being able to handle paths containing ':'?
I'd prefer an approach that doesn't barf on special characters in file names. Maybe \n is already a lost cause, but other than that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the path separator here comes from gdb, not from me. Using colons in directory names in Linux is already heavily frowned upon because you can't e.g. add them to PATH. I don't think we'll regret this.
src/SourcesCommand.cc
Outdated
FATAL() << "Failed to wait on gdb script host"; | ||
} | ||
|
||
if (!(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird! why can't we just communicate over the pipe like a regular program?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit afraid this will interact poorly with ctrl-Z job control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm yeah. I was in the mindset of writing kernel behavior tests :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to change this? I don't like it :-)
src/SourcesCommand.cc
Outdated
FATAL() << "Failed to wait on gdb script host"; | ||
} | ||
|
||
if (!(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to change this? I don't like it :-)
…ths in the `sources` command.
6ea338f
to
a0ecb21
Compare
This is relatively straightforward, just ugly. One notable difference in the behavior from gdb is that when multiple paths are provided gdb will try them all in sequence but this code just tries the last one. That works for the use case that's motivating this PR but may cause issues down the line.
I'm reasonably confident I didn't regress anything but there's no automated testing for
rr sources
.