Skip to content

Commit

Permalink
Use original accessor to access unreified instance vars. Fix #4455
Browse files Browse the repository at this point in the history
The invokedynamic logic for accessing instance variables did not
account for field-based instance variables that shift table-
based variables off in the "logical" index into the object. As a
result, once bound the index into the table was used as the
logical index, resulting in overridden BasicObject.getVariable
in the reified subclasses causing the fields to be accessed
instead.

This patch modifies indy binding for table-based variables to
use the original accessor rather than attempting to get the values
from the object directly.

The problem in #4455 was that the @opts variable holding options
for the dataset was bound to logical index 1 based on table index
1, resulting in the second of three reified instance variables
being accessed instead.

I will run a few more scenarios from #4455 to ensure those cases
are fixed as well.

The instance variable subsystem needs a bit of an overhaul. The
base RubyBasicObject.getVariable expects a table offset while the
overridden versions expect a logical offset. The binding via the
accessors is not as direct as it could be, but the accessors
contain logic that is hard to do in the direct path. And the
methods involved in managing instance variables and reified ivars
needs a more complete set of unit tests in Java to ensure they are
working properly together.
headius committed Feb 23, 2017
1 parent ff996fb commit 46ca986
Showing 1 changed file with 6 additions and 7 deletions.
13 changes: 6 additions & 7 deletions core/src/main/java/org/jruby/ir/targets/Bootstrap.java
Original file line number Diff line number Diff line change
@@ -412,7 +412,6 @@ private static MethodHandle createAttrReaderHandle(InvokeSite site, IRubyObject

MethodHandle getValue;

// FIXME: Duplicated from ivar get
if (accessor instanceof FieldVariableAccessor) {
int offset = ((FieldVariableAccessor)accessor).getOffset();
getValue = Binder.from(site.type())
@@ -424,9 +423,9 @@ private static MethodHandle createAttrReaderHandle(InvokeSite site, IRubyObject
getValue = Binder.from(site.type())
.drop(0, 2)
.filterReturn(filter)
.cast(methodType(Object.class, RubyBasicObject.class))
.append(accessor.getIndex())
.invokeVirtualQuiet(LOOKUP, "getVariable");
.cast(methodType(Object.class, Object.class))
.prepend(accessor)
.invokeVirtualQuiet(LOOKUP, "get");
}

// NOTE: Must not cache the fully-bound handle in the method, since it's specific to this class
@@ -459,9 +458,9 @@ private static MethodHandle createAttrWriterHandle(InvokeSite site, IRubyObject
setValue = Binder.from(site.type())
.drop(0, 2)
.filterReturn(filter)
.cast(methodType(void.class, RubyBasicObject.class, Object.class))
.insert(1, accessor.getIndex())
.invokeVirtualQuiet(LOOKUP, "setVariable");
.cast(methodType(void.class, Object.class, Object.class))
.prepend(accessor)
.invokeVirtualQuiet(LOOKUP, "set");
}

return setValue;

0 comments on commit 46ca986

Please sign in to comment.