[Truffle] Add an automatic check for missing lowerFixnum annotations.

* Rename check_ambiguous_arguments to check_dsl_usage as it's now more general.
jruby · Dec 16, 2016 · e1f213d · e1f213d
1 parent b720575
commit e1f213d
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 9 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -24,7 +24,7 @@ matrix:
   - env: JT='test gems' COMMAND=test/truffle/gems/install-gems.sh
   - env: JT='test ecosystem' JAVA_OPTS="$JAVA_OPTS -Xmx512m" HAS_REDIS=true COMMAND=test/truffle/ecosystem-travis-install.sh
   - env: JT='test tck' COMMAND='mx -v build' SKIP_BUILD=true
-  - env: JT='check_ambiguous_arguments' SKIP_BUILD=true V=1
+  - env: JT='check_dsl_usage' SKIP_BUILD=true V=1
   - env: JT='test mri'
   - env: JT='test bundle'
   # Exlude the default job https://github.com/travis-ci/travis-ci/issues/4681

diff --git a/tool/jt.rb b/tool/jt.rb
@@ -1298,9 +1298,9 @@ def next(*args)
     puts `cat spec/truffle/tags/core/**/**.txt | grep 'fails:'`.lines.sample
   end
 
-  def check_ambiguous_arguments
+  def check_dsl_usage
     clean
-    # modify pom
+    # modify pom to add -parameters for javac
     pom = "#{JRUBY_DIR}/truffle/pom.rb"
     contents = File.read(pom)
     contents.sub!(/^(\s+)('-J-Dfile.encoding=UTF-8')(.+\n)(?!\1'-parameters')/) do
@@ -1309,7 +1309,7 @@ def check_ambiguous_arguments
     File.write pom, contents
 
     build
-    run({ "TRUFFLE_CHECK_AMBIGUOUS_OPTIONAL_ARGS" => "true" }, '-e', 'exit')
+    run({ "TRUFFLE_CHECK_DSL_USAGE" => "true" }, '-e', 'exit')
   end
 
 end

diff --git a/truffle/src/main/java/org/jruby/truffle/builtins/AmbiguousOptionalArgumentChecker.java b/truffle/src/main/java/org/jruby/truffle/builtins/AmbiguousOptionalArgumentChecker.java
@@ -17,8 +17,12 @@
 public class AmbiguousOptionalArgumentChecker {
 
     public static boolean SUCCESS = true;
+    private static boolean AVAILABLE = true;
 
     public static void verifyNoAmbiguousOptionalArguments(CoreMethodNodeManager.MethodDetails methodDetails) {
+        if (!AVAILABLE) {
+            return;
+        }
         try {
             verifyNoAmbiguousOptionalArgumentsWithReflection(methodDetails);
         } catch (Exception e) {
@@ -54,8 +58,9 @@ private static void verifyNoAmbiguousOptionalArgumentsWithReflection(CoreMethodN
                         Parameter parameter = parameters[n];
                         boolean isNamePresent = parameter.isNamePresent();
                         if (!isNamePresent) {
+                            AVAILABLE = SUCCESS = false;
                             System.err.println("Method parameters names are not available for " + method);
-                            System.exit(1);
+                            return;
                         }
                         String name = parameter.getName();
 

diff --git a/truffle/src/main/java/org/jruby/truffle/builtins/CoreMethodNodeManager.java b/truffle/src/main/java/org/jruby/truffle/builtins/CoreMethodNodeManager.java
@@ -54,7 +54,7 @@
 
 public class CoreMethodNodeManager {
 
-    private static final boolean CHECK_AMBIGUOUS_OPTIONAL_ARGS = System.getenv("TRUFFLE_CHECK_AMBIGUOUS_OPTIONAL_ARGS") != null;
+    private static final boolean CHECK_DSL_USAGE = System.getenv("TRUFFLE_CHECK_DSL_USAGE") != null;
     private final RubyContext context;
     private final SingletonClassNode singletonClassNode;
     private final PrimitiveManager primitiveManager;
@@ -191,7 +191,7 @@ private static CallTarget makeGenericMethod(RubyContext context, MethodDetails m
 
         final RubyNode methodNode = createCoreMethodNode(context, sourceSection, methodDetails.getNodeFactory(), method);
 
-        if (CHECK_AMBIGUOUS_OPTIONAL_ARGS) {
+        if (CHECK_DSL_USAGE) {
             AmbiguousOptionalArgumentChecker.verifyNoAmbiguousOptionalArguments(methodDetails);
         }
 
@@ -234,10 +234,15 @@ public static RubyNode createCoreMethodNode(RubyContext context, SourceSection s
         final int optional = method.optional();
         final int nArgs = required + optional;
 
+        if (CHECK_DSL_USAGE) {
+            LowerFixnumChecker.checkLowerFixnumArguments(nodeFactory, needsSelf ? 1 : 0, method);
+        }
+
         for (int n = 0; n < nArgs; n++) {
             RubyNode readArgumentNode = new ProfileArgumentNode(new ReadPreArgumentNode(n, MissingArgumentBehavior.UNDEFINED));
             argumentsNodes.add(transformArgument(context, sourceSection, method, readArgumentNode, n + 1));
         }
+
         if (method.rest()) {
             argumentsNodes.add(new ReadRemainingArgumentsNode(nArgs));
         }
@@ -363,8 +368,10 @@ public static boolean isSafe(RubyContext context, UnsafeGroup[] groups) {
     }
 
     public void allMethodInstalled() {
-        if (CHECK_AMBIGUOUS_OPTIONAL_ARGS && !AmbiguousOptionalArgumentChecker.SUCCESS) {
-            System.exit(1);
+        if (CHECK_DSL_USAGE) {
+            if (!(AmbiguousOptionalArgumentChecker.SUCCESS && LowerFixnumChecker.SUCCESS)) {
+                System.exit(1);
+            }
         }
     }
 

diff --git a/truffle/src/main/java/org/jruby/truffle/builtins/LowerFixnumChecker.java b/truffle/src/main/java/org/jruby/truffle/builtins/LowerFixnumChecker.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. This
+ * code is released under a tri EPL/GPL/LGPL license. You can use it,
+ * redistribute it and/or modify it under the terms of the:
+ *
+ * Eclipse Public License version 1.0
+ * GNU General Public License version 2
+ * GNU Lesser General Public License version 2.1
+ */
+package org.jruby.truffle.builtins;
+
+import com.oracle.truffle.api.dsl.Cached;
+import com.oracle.truffle.api.dsl.NodeFactory;
+import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.frame.VirtualFrame;
+
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
+
+import org.jruby.truffle.core.array.ArrayUtils;
+import org.jruby.truffle.language.RubyNode;
+
+public class LowerFixnumChecker {
+
+    public static boolean SUCCESS = true;
+
+    public static void checkLowerFixnumArguments(NodeFactory<? extends RubyNode> nodeFactory, int initialSkip, CoreMethod methodAnnotation) {
+        final Class<? extends RubyNode> nodeClass = nodeFactory.getNodeClass();
+        byte[] lowerArgs = null;
+        for (Method specialization : nodeClass.getDeclaredMethods()) {
+            if (specialization.isAnnotationPresent(Specialization.class)) {
+                Class<?>[] argumentTypes = specialization.getParameterTypes();
+                int skip = initialSkip;
+                if (argumentTypes.length > 0 && argumentTypes[0] == VirtualFrame.class) {
+                    skip++;
+                }
+                int end = argumentTypes.length;
+                Annotation[][] annos = specialization.getParameterAnnotations();
+                for (int i = end - 1; i >= skip; i--) {
+                    boolean cached = false;
+                    for (Annotation anno : annos[i]) {
+                        cached |= anno instanceof Cached;
+                    }
+                    if (cached) {
+                        end--;
+                    } else {
+                        break;
+                    }
+                }
+                if (lowerArgs == null) {
+                    lowerArgs = new byte[end - skip];
+                } else {
+                    assert lowerArgs.length == end - skip;
+                }
+                for (int i = skip; i < end; i++) {
+                    Class<?> argumentType = argumentTypes[i];
+                    if (argumentType == int.class) {
+                        lowerArgs[i - skip] |= 0b01;
+                    } else if (argumentType == long.class) {
+                        lowerArgs[i - skip] |= 0b10;
+                    }
+                }
+            }
+        }
+
+        // Verify against the lowerFixnum annotation
+        final int[] lowerFixnum = methodAnnotation.lowerFixnum();
+        for (int i = 0; i < lowerArgs.length; i++) {
+            boolean shouldLower = lowerArgs[i] == 0b01; // int without long
+            if (shouldLower && !ArrayUtils.contains(lowerFixnum, i + 1)) {
+                SUCCESS = false;
+                System.err.println("Node " + nodeFactory.getNodeClass().getCanonicalName() + " should use lowerFixnum for argument " + (i + 1));
+            }
+        }
+    }
+}