Merge pull request #5358 from headius/toplevel_visibility_in_jit

headius · web-flow · commit ba9583ff8998 · 2018-10-11T18:10:35.000-05:00
Fix toplevel method visibility in jit/fullint script scopes.
diff --git a/core/src/main/java/org/jruby/ir/instructions/PushMethodFrameInstr.java b/core/src/main/java/org/jruby/ir/instructions/PushMethodFrameInstr.java
@@ -6,27 +6,34 @@
 import org.jruby.ir.persistence.IRReaderDecoder;
 import org.jruby.ir.transformations.inlining.CloneInfo;
 import org.jruby.ir.transformations.inlining.SimpleCloneInfo;
+import org.jruby.runtime.Visibility;
 
 public class PushMethodFrameInstr extends NoOperandInstr implements FixedArityInstr {
     private final RubySymbol frameName;
+    private final Visibility visibility;
     
-    public PushMethodFrameInstr(RubySymbol frameName) {
+    public PushMethodFrameInstr(RubySymbol frameName, Visibility visibility) {
         super(Operation.PUSH_METHOD_FRAME);
 
         this.frameName = frameName;
+        this.visibility = visibility;
     }
 
     public RubySymbol getFrameName() {
         return frameName;
     }
 
+    public Visibility getVisibility() {
+        return visibility;
+    }
+
     @Override
     public Instr clone(CloneInfo ii) {
         return ii instanceof SimpleCloneInfo ? this : NopInstr.NOP;  // FIXME: Is this correct?
     }
 
     public static PushMethodFrameInstr decode(IRReaderDecoder d) {
-        return new PushMethodFrameInstr(d.decodeSymbol());
+        return new PushMethodFrameInstr(d.decodeSymbol(), Visibility.values()[d.decodeByte()]);
     }
 
     @Override
diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -16,6 +16,7 @@
 import org.jruby.ir.instructions.NonlocalReturnInstr;
 import org.jruby.ir.instructions.PopBlockFrameInstr;
 import org.jruby.ir.instructions.PushBlockFrameInstr;
+import org.jruby.ir.instructions.PushMethodFrameInstr;
 import org.jruby.ir.instructions.ReceiveArgBase;
 import org.jruby.ir.instructions.ReceivePostReqdArgInstr;
 import org.jruby.ir.instructions.ReceivePreReqdArgInstr;
@@ -383,10 +384,7 @@ protected static void processBookKeepingOp(ThreadContext context, Block block, I
                 break;
             case PUSH_METHOD_FRAME:
                 context.preMethodFrameOnly(implClass, name, self, blockArg);
-                // Only the top-level script scope has PRIVATE visibility.
-                // This is already handled as part of Interpreter.execute above.
-                // Everything else is PUBLIC by default.
-                context.setCurrentVisibility(Visibility.PUBLIC);
+                context.setCurrentVisibility(((PushMethodFrameInstr) instr).getVisibility());
                 break;
             case PUSH_BACKREF_FRAME:
                 context.preBackrefMethod();
diff --git a/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java b/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java
@@ -11,6 +11,7 @@
 import org.jruby.ir.operands.Variable;
 import org.jruby.ir.representations.BasicBlock;
 import org.jruby.ir.representations.CFG;
+import org.jruby.runtime.Visibility;
 
 import java.util.ListIterator;
 
@@ -135,7 +136,9 @@ public Object execute(IRScope scope, Object... data) {
                     if (scope.needsOnlyBackref()) {
                         entryBB.addInstr(new PushBackrefFrameInstr());
                     } else {
-                        entryBB.addInstr(new PushMethodFrameInstr(scope.getName()));
+                        entryBB.addInstr(new PushMethodFrameInstr(
+                                scope.getName(),
+                                scope.isScriptScope() ? Visibility.PRIVATE : Visibility.PUBLIC));
                     }
                 }
                 if (requireBinding) entryBB.addInstr(new PushMethodBindingInstr());
diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1770,7 +1770,7 @@ public void PushMethodFrameInstr(PushMethodFrameInstr pushframeinstr) {
         // FIXME: this should be part of explicit call protocol only when needed, optimizable, and correct for the scope
         // See also CompiledIRMethod.call
         jvmMethod().loadContext();
-        jvmAdapter().getstatic(p(Visibility.class), "PUBLIC", ci(Visibility.class));
+        jvmAdapter().getstatic(p(Visibility.class), pushframeinstr.getVisibility().name(), ci(Visibility.class));
         jvmAdapter().invokevirtual(p(ThreadContext.class), "setCurrentVisibility", sig(void.class, Visibility.class));
     }
 

Original file line number	Diff line number	Diff line change
`@@ -1770,7 +1770,7 @@ public void PushMethodFrameInstr(PushMethodFrameInstr pushframeinstr) {`
`1770`	`1770`	`// FIXME: this should be part of explicit call protocol only when needed, optimizable, and correct for the scope`
`1771`	`1771`	`// See also CompiledIRMethod.call`
`1772`	`1772`	`jvmMethod().loadContext();`
`1773`		`- jvmAdapter().getstatic(p(Visibility.class), "PUBLIC", ci(Visibility.class));`
	`1773`	`+ jvmAdapter().getstatic(p(Visibility.class), pushframeinstr.getVisibility().name(), ci(Visibility.class));`
`1774`	`1774`	`jvmAdapter().invokevirtual(p(ThreadContext.class), "setCurrentVisibility", sig(void.class, Visibility.class));`
`1775`	`1775`	`}`
`1776`	`1776`