Use caller method name from ruby stack #78
Conversation
as well as determining whether or not it was an instance method. The reason for this is that the rotoscope stack isn't as reliable.
|
|
||
| rs_callsite_t c_callsite(rb_trace_arg_t *trace_arg) { | ||
| VALUE path = rb_tracearg_path(trace_arg); | ||
| int line; | ||
| VALUE frame = caller_frame(&line, false); |
There was a problem hiding this comment.
You are using pass-by-reference essentially to be able to return two things from caller_frame, the frame and the line. Correct?
| return (rs_callsite_t){ | ||
| .filepath = path, .lineno = FIX2INT(rb_tracearg_lineno(trace_arg)), | ||
| .filepath = rb_tracearg_path(trace_arg), | ||
| .lineno = FIX2INT(rb_tracearg_lineno(trace_arg)), |
There was a problem hiding this comment.
is this different from line?
There was a problem hiding this comment.
Good question. I tried to continue using the tracepoint functions (i.e. rb_tracearg_*) to minimize how much is changing at once. However, I do think they should be the same and would like to have this file only use rb_profile_frames in a follow-up PR.
| .filepath = rb_tracearg_path(trace_arg), | ||
| .lineno = FIX2INT(rb_tracearg_lineno(trace_arg)), | ||
| .method_name = rb_profile_frame_method_name(frame), | ||
| .singleton_p = rb_profile_frame_singleton_method_p(frame), | ||
| }; | ||
| } | ||
|
|
||
| rs_callsite_t ruby_callsite() { |
There was a problem hiding this comment.
Can you add a minimal comment explaining the difference between ruby_callsite and c_callsite?
There was a problem hiding this comment.
I would like to get rid of them as two separate code paths, but I think the bool ruby_call parameter still deserves documenting.
The reason they diverged historically was because rb_tracearg_ API for the callsite was working for C but not ruby. That tracepoint API was trying to returning the code location just after the call (i.e. inside the called function) but it would also skip non-ruby stack frames. rb_profile_frames replaced some earlier dirty hack we had isolated to ruby_callsite and allows us to get more than just the current code location, allowing us to use index 0 to continue getting the correct location for C but 1 to skip a frame and get the caller location for ruby.
There was a problem hiding this comment.
Added a comment to caller_frame on this key difference.
| if call.caller_method_name.nil? | ||
| caller_class_name = '<ROOT>' |
There was a problem hiding this comment.
How is this change related?
There was a problem hiding this comment.
When tracing just starts, we won't know the caller class name and didn't know the caller method name. However, now that we are using the ruby stack to get the caller method name, these two are orthogonal, so we can assume that just because we don't have a caller method name that we do have a class name. As such, I needed to move this out of this conditional block.
The rename of the default value isn't as significant. It is just a bit confusing to say that it is the root when thinking in the context of a ruby stack, so '<UNKNOWN>' seems like a more appropriate default value.
| // causes the start argument to effectively always | ||
| // act as if it were 0, so we need to also get the top | ||
| // frame. (https://bugs.ruby-lang.org/issues/14607) | ||
| rb_profile_frames(0, frame_index + 1, frames, lines); |
There was a problem hiding this comment.
For future reference: rb_profile_frames is currently undocumented, but appears ina a header file https://github.com/ruby/ruby/blob/6f2c516d8031e1c7f4704bd59c5349fab7f24187/include/ruby/debug.h#L26
|
|
Problem
We have been seeing more problems with the caller location not matching the entity and method name, beyond the issues already reported in #57 and #59.
Solution
Since these problems seem to stem from trying to keep a mirror of the stack in sync with the ruby stack, we should just try to rely on the ruby stack itself. This PR does that, but using rb_profile_frames to get the method name and to check if it is a singleton method.
There is a notable change to the tests due to rb_profile_frames skipping C frames to get the next ruby frame. This means that calls from C code show up as calls to the ruby code that called that C code. However, this is also more consistent with the filename and line number that was being used for the caller.
If this makes sense, then I think we should also deprecate the caller entity then remove that in a follow-up PR.
bin/fmtwas successfully run