Include overriding classes in backtrace mapping#6653
Merged
headius merged 6 commits intojruby:masterfrom May 19, 2021
Merged
Conversation
Our Ruby backtrace-generation logic depends on a mapping of class and method names to their equivalent Ruby name, but this fails when the JRubyMethod annotation is on an abstract method implemented by one or more subclasses. The subclasses are not registered in the mapping and when their overridden methods appear in the Java stack trace they will not be replaced with their Ruby equivalent. This patch adds support for JRubyMethod.implementers, a list of additional classes to include in the mapping table. This is a naive change currently, in that it assumes all methods on class A should also be mapped for class B < A regardless of whether they are actually overridden or not. This is a first pass at fixing the missing trace lines reported in issue jruby#5200.
One call to addBoundMethods per self class and overriding classes.
headius
force-pushed
the
abstract_jruby_method_backtrace
branch
from
cc0048a to
6470090
Compare
The old logic was attempting to add all Ruby method names for a given Java name but since we can only store one value the last name would win. This modifies the logic to only attempt to add the first name (or the simple name of the method, if no others are specified) to the table.
After some investigation it is clear that the "smart" approach of finding all subclasses that override a given method is infeasible: * Neither the Java compiler API nor the reflection API provide a way to walk all subclasses of a given class. * Searching for overrides would trigger exceptions for each checked signature that is not overridden. And since having the subclasses listed on each bound method would be very fragile, this commit moves that list to the class level on the JRubyClass annotation (if present) and simply duplicates the parent's mapping list for each specified child. Children can still have additional bindings that do not appear in the parent's set.
* RubyInteger is overridden by RubyFixnum and RubyBignum * AbstractRubyMethod is overriden by RubyMethod and RubyUnboundMethod * RubyArray is implemented by RubyArrayOneObject, RubyArrayTwoObject, and StringArraySet
Member
Author
|
@enebo This is green and done enough for now. It fixes the cases of Method/UnboundMethod, Integer/Fixnum/Bignum, and Array specialization overrides. It does not fix other "fast path" methods like RubyFixnum.op_plus_one but for that we would need to rethink how we annotate methods, probably by splitting binding and backtrace support into separate annotations. This does fix the specific case called out in #5200. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR will fix #5200 by also adding subclasses' bound methods to the mapping table, in one way or another. The initial logic adds a JRubyMethod.implementers list of subclasses to include in the mapping table, but it is fairly naive and does not check if the method is actually overridden. The fix provided would be sufficient, but requires at least one annotation in a given native class to specify a list of implementers to include.
If other experiments don't pan out, we can go with this, but I will try to refine this to only include the actually-overridden methods, and ideally to not require an explicit list of classes.