Correctly locate filepath of caller to tracepoint #13
Conversation
jahfer
force-pushed
the
find-callsite-and-callpoint
branch
2 times, most recently
from
2aff9c3
to
00b3b69
Compare
jahfer
changed the title
jahfer
force-pushed
the
find-callsite-and-callpoint
branch
4 times, most recently
from
df8257f
to
b2a5f96
Compare
jahfer
force-pushed
the
find-callsite-and-callpoint
branch
5 times, most recently
from
899114a
to
4471438
Compare
jahfer
force-pushed
the
find-callsite-and-callpoint
branch
from
4471438
to
c42c834
Compare
ext/rotoscope/rotoscope.c
Outdated
| // drop down to default on purpose | ||
| default: | ||
| caller_str = (caller_str == NULL) ? (caller_location(1) + 1) : caller_str; | ||
| if (caller_str != NULL) |
There was a problem hiding this comment.
From the above statement, it seems like the else branch would never be hit? Or can caller_location(1) + 1 somehow end up being NULL?
There was a problem hiding this comment.
This would be NULL if the execution stack was length 1, which seems…unlikely? Grabbing an artifact from a CI run using this, I don't see any filepaths that get set to the else branch, so I will remove it 👍
ext/rotoscope/rotoscope.c
Outdated
| caller_str = strtok(NULL, ":"); | ||
| if (caller_str) | ||
| { | ||
| callsite.lineno = (unsigned int)strtol(caller_str, &endptr, 10); |
There was a problem hiding this comment.
Since you're not actually using endptr for anything, I think you can just pass NULL.
ext/rotoscope/rotoscope.c
Outdated
| caller_str = (caller_str == NULL) ? (caller_location(1) + 1) : caller_str; | ||
| if (caller_str != NULL) | ||
| { | ||
| char *data = strdup(caller_str); |
There was a problem hiding this comment.
This is leaking memory, if that's of concern
There was a problem hiding this comment.
This was my lousy stackoverflow debugging of a segfault in CI with strtok. I don't think it's actually needed, so I'll get rid of this as well.
ext/rotoscope/rotoscope.c
Outdated
| caller_str = strtok(NULL, ":"); | ||
| if (caller_str) | ||
| { | ||
| callsite.lineno = (unsigned int)strtol(caller_str, NULL, 10); |
There was a problem hiding this comment.
Not sure if you'd like to set this to zero otherwise, in the rare (maybe "impossible"?) case where there's no line number in the caller string (which would result in "random" line numbers).
jahfer
force-pushed
the
find-callsite-and-callpoint
branch
7 times, most recently
from
a022695
to
e30f356
Compare
| { | ||
| strncpy((char *)callsite.filepath, caller_str, sizeof(callsite.filepath)); | ||
| strtok((char *)callsite.filepath, ":"); | ||
| if (caller_str = strtok(NULL, ":")) |
There was a problem hiding this comment.
after lots of back-and-forth here, I've settled on two invocations of strtok instead of strchr since there's some extra bounds checking done by strtok that I don't want to have to reimplement.
tl;dr I got a segfault and gave up on strchr
jahfer
force-pushed
the
find-callsite-and-callpoint
branch
from
e30f356
to
1a76b68
Compare
This PR will correctly infer the original caller of the method traced via
Kernel.caller_locations.Unfortunately, this introduces a major decrease in performance (a 10-minute 96-way CI run now takes > 1 hour). As a minor optimization, we're able to shortcut this expensive call for methods defined in C using
rb_tracearg_pathsinceKernel.caller_locationscannot point to the C-code for its definition.The only way to get a performant solution would be to have some traction on Bug #7976 in Ruby. Research into different methods of path detection have been noted in this gist: https://gist.github.com/jahfer/092e44df6f15298bae3cf47056301708