DynComp fixes

java/daikon/chicory/Runtime.java

@@ -939,9 +939,11 @@ public Class<?> primitiveClass() {
     }
   }
 
+  /** The major version of the running JVM: 8 for Java 8, 24 for Java 24, and so on. */
+  private static final int javaMajorVersion = javaMajorVersion(System.getProperty("java.version"));
+
   /** True if the running JVM is for Java 9 or later. */
-  private static final boolean isJava9orLater =
-      !System.getProperty("java.version").startsWith("1.");
+  private static final boolean isJava9orLater = javaMajorVersion >= 9;
 
   /**
    * Returns true if the running JVM is for Java 9 or later.
@@ -953,10 +955,7 @@ public static boolean isJava9orLater() {
   }
 
   /** True if the running JVM is for Java 24 or later. */
-  private static final boolean isJava24orLater =
-      !System.getProperty("java.version").startsWith("1.")
-          && !System.getProperty("java.version").startsWith("9.")
-          && Integer.parseInt(System.getProperty("java.version").substring(0, 2)) >= 24;
+  private static final boolean isJava24orLater = javaMajorVersion >= 24;
 
   /**
    * Returns true if the running JVM is for Java 24 or later.
@@ -967,6 +966,43 @@ public static boolean isJava24orLater() {
     return isJava24orLater;
   }
 
+  /**
+   * Returns the major version encoded in a {@code java.version} property value: 8 for Java 8, 24
+   * for Java 24, and so on.
+   *
+   * <p>Both version schemes are accepted: the pre-Java-9 {@code "1.8.0_432"} form, whose major
+   * version is its second component, and the Java 9 and later {@code "24"}, {@code "24.0.1"}, and
+   * {@code "24-ea"} forms, whose major version is the first. In the latter scheme the major version
+   * may stand alone with no separator at all, as it does for a GA release such as {@code "9"}.
+   *
+   * <p>Only the leading digits are examined; anything after them is ignored rather than rejected,
+   * so {@code "9foo"} yields 9. That is deliberate. This runs from a static initializer, so
+   * throwing on an unrecognized suffix would turn an unanticipated vendor version string into an
+   * ExceptionInInitializerError inside an instrumented program, which is the failure this method
+   * exists to prevent. Ignoring the suffix instead yields the right major version for any string
+   * that begins with one. A value with no leading digits at all is still rejected.
+   *
+   * @param version the value of the {@code java.version} system property
+   * @return the major version it encodes
+   */
+  // Package-private rather than private so that RuntimeTest can exercise it directly; the value
+  // derived from the running JVM is fixed at class-initialization time and cannot be varied.
+  static int javaMajorVersion(String version) {
+    // Java 8 and earlier report "1.N..."; the major version is the second component.
+    String rest = version.startsWith("1.") ? version.substring(2) : version;
+    // The major version is the leading run of digits.  What follows it is "." for a release with
+    // minor components, "-" or "+" for a pre-release or build identifier, and nothing at all for a
+    // bare GA release such as "9".
+    int end = 0;
+    while (end < rest.length() && Character.isDigit(rest.charAt(end))) {
+      end++;
+    }
+    if (end == 0) {
+      throw new IllegalArgumentException("Cannot parse java.version: " + version);
+    }
+    return Integer.parseInt(rest.substring(0, end));
+  }
+
   // ///////////////////////////////////////////////////////////////////////////
   // Copied code
   //

java/daikon/chicory/RuntimeTest.java

@@ -0,0 +1,50 @@
+package daikon.chicory;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link daikon.chicory.Runtime}. */
+@RunWith(JUnit4.class)
+public class RuntimeTest {
+
+  /**
+   * Tests {@link Runtime#javaMajorVersion}. The values are real {@code java.version} strings; the
+   * bare "9" is the one that matters most, because Java 9 GA reported its version with no separator
+   * after the major number.
+   */
+  @Test
+  public void testJavaMajorVersion() {
+    // Java 8 and earlier: the major version is the second component.
+    assertEquals(8, Runtime.javaMajorVersion("1.8.0_432"));
+    assertEquals(7, Runtime.javaMajorVersion("1.7.0_80"));
+    // Java 9 and later, with minor components.
+    assertEquals(9, Runtime.javaMajorVersion("9.0.4"));
+    assertEquals(11, Runtime.javaMajorVersion("11.0.28"));
+    assertEquals(24, Runtime.javaMajorVersion("24.0.1"));
+    assertEquals(25, Runtime.javaMajorVersion("25.0.2"));
+    // Java 9 and later, major version alone.
+    assertEquals(9, Runtime.javaMajorVersion("9"));
+    assertEquals(24, Runtime.javaMajorVersion("24"));
+    // Pre-release and build identifiers.
+    assertEquals(9, Runtime.javaMajorVersion("9-ea"));
+    assertEquals(26, Runtime.javaMajorVersion("26-ea"));
+    assertEquals(26, Runtime.javaMajorVersion("26+11"));
+
+    // Trailing junk after the major version is ignored rather than rejected; this is deliberate,
+    // and javaMajorVersion's javadoc says why.  A value with no leading digits still throws.
+    assertEquals(9, Runtime.javaMajorVersion("9foo"));
+    assertThrows(IllegalArgumentException.class, () -> Runtime.javaMajorVersion("bogus"));
+  }
+
+  /** Tests that {@link Runtime#isJava24orLater} agrees with the JVM running the test. */
+  @Test
+  public void testIsJava24orLater() {
+    int major = Runtime.javaMajorVersion(System.getProperty("java.version"));
+    assertEquals(major >= 24, Runtime.isJava24orLater());
+    assertEquals(major >= 9, Runtime.isJava9orLater());
+  }
+}

java/daikon/dcomp/ClassGen24.java

@@ -64,9 +64,6 @@ public class ClassGen24 {
   /** True if this class is an interface. */
   private final boolean isInterface;
 
-  /** True if this class is static. */
-  private final boolean isStatic;
-
   /**
    * Creates a ClassGen24 object.
    *
@@ -85,7 +82,6 @@ public ClassGen24(
 
     accessFlags = classModel.flags();
     isInterface = accessFlags.has(AccessFlag.INTERFACE);
-    isStatic = accessFlags.has(AccessFlag.STATIC);
 
     superclassName = getSuperclassName(classModel);
 
@@ -147,15 +143,6 @@ public final boolean isInterface() {
     return isInterface;
   }
 
-  /**
-   * Returns true if this class is static.
-   *
-   * @return true if this class is static
-   */
-  public final boolean isStatic() {
-    return isStatic;
-  }
-
   /**
    * Returns this class's name, in binary format.
    *

java/daikon/dcomp/DCInstrument.java

@@ -734,6 +734,11 @@ public JavaClass instrument() {
 
     boolean junit_test_class = false;
 
+    // Skipped for JDK classes.  A JDK class is never a JUnit test class: the check below confirms
+    // one only by a junit.framework.TestCase superclass or an org/junit/Test annotation.  Skipping
+    // them loses no state transition -- STARTING and TEST_DISCOVERY are re-evaluated on the next
+    // class load, and the test classes themselves are never in the JDK -- and it keeps a
+    // getStackTrace, plus TEST_DISCOVERY's superclass walk, off the JDK class-loading path.
     if (!in_jdk) {
       // A very tricky special case: If JUnit is running and the current
       // class has been passed to JUnit on the command line, then this
@@ -1043,23 +1048,39 @@ public JavaClass instrument() {
                 "DynComp warning: ClassFile: %s - method %s has too many bytecodes to instrument"
                     + " and is being skipped.%n",
                 classname, mgen.getName());
-            // Build a dummy instrumented method that has DCompMarker
-            // parameter and no instrumentation.
-            // first, restore unmodified method
-            mgen = new MethodGen(m, classname, pool);
-            // restore StackMapTable
-            setCurrentStackMapTable(mgen, classGen.getMajor());
-            // Add the DCompMarker parameter
-            add_dcomp_param(mgen);
-            remove_local_variable_type_table(mgen);
-            // try again
+            // First, restore the unmodified method, to recover its original signature.
+            MethodGen original = new MethodGen(m, classname, pool);
             if (replacingMethod) {
+              // The method is replaced rather than duplicated and keeps its own signature, so
+              // there is no uninstrumented copy to forward to and its callers use the
+              // uninstrumented calling convention.  Emitting the original body is then correct.
+              debugInstrument.log(
+                  "Copying oversized method without instrumentation: %s%n", original.getName());
+              debugInstrument.indent();
+              mgen = original;
+              setCurrentStackMapTable(mgen, classGen.getMajor());
+              // No add_dcomp_param call here: it returns early for main and <clinit>.  For the
+              // remaining case -- a JUnit test class -- it would append the marker and alter the
+              // descriptor, so omit the call to preserve JUnit discovery.  That matches the normal
+              // path above, which adds the marker only if !junit_test_class.
+              remove_local_variable_type_table(mgen);
               classGen.replaceMethod(m, mgen.getMethod());
               if (BcelUtil.isMain(mgen)) {
                 classGen.addMethod(create_dcomp_stub(mgen).getMethod());
               }
+              debugInstrument.exdent();
+              debugInstrument.log("End of copy%n");
             } else {
-              classGen.addMethod(mgen.getMethod());
+              // Emit a method with the DCompMarker parameter that forwards to the uninstrumented
+              // copy of this method and maintains the tag stack; see
+              // create_oversized_method_stub.
+              debugInstrument.log(
+                  "Oversized method, creating stub that forwards to uninstrumented version: %s%n",
+                  original.getName());
+              debugInstrument.indent();
+              classGen.addMethod(create_oversized_method_stub(original).getMethod());
+              debugInstrument.exdent();
+              debugInstrument.log("End of stub%n");
             }
           } else {
             throw e;
@@ -1317,17 +1338,17 @@ public JavaClass instrument_jdk_class() {
                 "DynComp warning: ClassFile: %s - method %s has too many bytecodes to instrument"
                     + " and is being skipped.%n",
                 classname, mgen.getName());
-            // Build a dummy instrumented method that has DCompMarker
-            // parameter and no instrumentation.
-            // first, restore unmodified method
+            // Emit a method with the DCompMarker parameter that forwards to the uninstrumented
+            // copy of this method and maintains the tag stack; see create_oversized_method_stub.
+            // First, restore the unmodified method, to recover its original signature.
             mgen = new MethodGen(m, classname, pool);
-            // restore StackMapTable
-            setCurrentStackMapTable(mgen, classGen.getMajor());
-            // Add the DCompMarker parameter
-            add_dcomp_param(mgen);
-            remove_local_variable_type_table(mgen);
-            // try again
-            classGen.addMethod(mgen.getMethod());
+            debugInstrument.log(
+                "Oversized method, creating stub that forwards to uninstrumented version: %s%n",
+                mgen.getName());
+            debugInstrument.indent();
+            classGen.addMethod(create_oversized_method_stub(mgen).getMethod());
+            debugInstrument.exdent();
+            debugInstrument.log("End of stub%n");
           } else {
             throw e;
           }
@@ -1338,10 +1359,7 @@ public JavaClass instrument_jdk_class() {
         if (debugInstrument.enabled) {
           t.printStackTrace();
         }
-        // TODO: Is it guaranteed that mgen is non-null by the time control reaches here?
-        if (mgen != null) {
-          skip_method(mgen);
-        }
+        skip_method(classname, m.getName());
         if (quit_if_error) {
           throw new Error("Error processing " + classname + "." + m.getName(), t);
         } else {
@@ -1472,7 +1490,19 @@ public void instrumentMethod(MethodGen mgen) {
    * @param m method to add to skipped_methods list
    */
   void skip_method(MethodGen m) {
-    skipped_methods.add(m.getClassName() + "." + m.getName());
+    skip_method(m.getClassName(), m.getName());
+  }
+
+  /**
+   * Adds the method name and containing class name to {@code skip_methods}, the list of
+   * uninstrumented methods. Use this overload where instrumentation may have failed before the
+   * method's {@link MethodGen} was built.
+   *
+   * @param classname the class that contains the method
+   * @param methodName the name of the method
+   */
+  void skip_method(String classname, String methodName) {
+    skipped_methods.add(classname + "." + methodName);
   }
 
   /**
@@ -4506,6 +4536,93 @@ boolean is_class_initialized_by_jvm(String classname) {
     return false;
   }
 
+  /**
+   * Returns a method, with the DCompMarker parameter added, whose body forwards to the
+   * uninstrumented copy of {@code mgen} and maintains the tag stack. Used for a method whose
+   * instrumented form would exceed the JVM's 64K code-size limit.
+   *
+   * <p>Such a method is still emitted with the DCompMarker parameter, so its callers use the
+   * calling convention for an instrumented method: they leave a tag on the tag stack for each
+   * primitive argument, expecting {@code DCRuntime.create_tag_frame} to pop them, and they expect a
+   * tag for a primitive result to have been pushed by the time it returns. A copy of the original
+   * body honors neither half of that contract -- the argument tags accumulate, and a caller of a
+   * primitive-returning method pops a tag that nobody pushed.
+   *
+   * <p>This is the "not instrumented" branch of {@link #handleInvoke} turned inside out: there, the
+   * caller discards the argument tags (via {@link #discard_primitive_tags}) and pushes the result
+   * tag because the callee will not; here, the callee does it because the caller assumes it will.
+   * Compare {@link #fix_native}, which does the same for a native method.
+   *
+   * @param mgen the unmodified method, with its original signature
+   * @return a method with the DCompMarker parameter that forwards to {@code mgen}
+   */
+  MethodGen create_oversized_method_stub(MethodGen mgen) {
+
+    InstructionList il = new InstructionList();
+    Type[] paramTypes = mgen.getArgumentTypes();
+    Type returnType = mgen.getReturnType();
+
+    // Discard the tag the caller pushed for each primitive argument; create_tag_frame would
+    // otherwise have popped them into the tag frame.
+    int primitive_cnt = 0;
+    for (Type paramType : paramTypes) {
+      if (paramType instanceof BasicType) {
+        primitive_cnt++;
+      }
+    }
+    if (primitive_cnt > 0) {
+      il.append(discard_tag_code(new NOP(), primitive_cnt));
+    }
+
+    // Load the receiver, if any, and then the original arguments.  The DCompMarker parameter is
+    // last, so the original arguments keep their slots and the marker is simply never loaded.
+    int offset = 0;
+    if (!mgen.isStatic()) {
+      il.append(InstructionFactory.createThis());
+      offset = 1;
+    }
+    for (Type paramType : paramTypes) {
+      il.append(InstructionFactory.createLoad(paramType, offset));
+      offset += paramType.getSize();
+    }
+
+    // Call the uninstrumented copy of this method.  INVOKESPECIAL rather than INVOKEVIRTUAL: we
+    // want this class's own copy, not an override in a subclass, which would run a different
+    // method's body -- and, if the subclass's copy is oversized too, would recur without bound.
+    il.append(
+        ifact.createInvoke(
+            mgen.getClassName(),
+            mgen.getName(),
+            returnType,
+            paramTypes,
+            mgen.isStatic() ? INVOKESTATIC : INVOKESPECIAL,
+            classGen.isInterface()));
+
+    // Push the tag for a primitive result that the instrumented body would have pushed on exit.
+    // Unlike fix_native and handleInvoke, this is done after the call rather than before it,
+    // so that a call that throws does not leave a tag behind: this stub has no exception handler to
+    // clean one up.
+    if (is_primitive(returnType)) {
+      il.append(dcr_call("push_const", CD_void, noArgsSig));
+    }
+    il.append(InstructionFactory.createReturn(returnType));
+
+    MethodGen stub =
+        new MethodGen(
+            mgen.getAccessFlags(),
+            returnType,
+            ArraysPlume.append(paramTypes, dcomp_marker),
+            ArraysPlume.append(mgen.getArgumentNames(), "marker"),
+            mgen.getName(),
+            mgen.getClassName(),
+            il,
+            pool);
+    stub.setMaxLocals();
+    stub.setMaxStack();
+
+    return stub;
+  }
+
   /**
    * Creates a method with a DcompMarker parameter that does nothing but call the corresponding
    * method without the DCompMarker argument. (Currently, only used for ? va main.)