Clojure

Incorrect bytecode generated for static methods on interfaces

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.9
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    JDK 9 or higher
  • Patch:
    Code
  • Approval:
    Triaged

Description

Calls to static interface methods compile on Java 8 but not on Java 9 because of a new bytecode restriction. As the usage of static interface methods increases, this problem will manifest more. I ran into it while using the AWS SDK (2.0 preview) which makes extensive usage of these methods.

public interface JDK8InterfaceMethods {
    // cannot call either of these from Clojure on Java 9
    // throws IncompatibleClassChangeError per JVMS
    public static long staticMethod0(long v) { return v; }
    public static String staticMethod1(String s) { return s; }
}

JVMS Section 5.4.3.3 and 5.4.3.4 describe method resolution
https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.3

The v2 patches have a full description in each commit. The basic approach is to link the method description correctly (a one-line change) but in order to do that we must emit JDK8 bytecode, not the current JDK5 bytecode. If this approach is decent I recommend getting in the change early in order to enjoy extended testing.

The original ticket had a repro here: https://github.com/ztellman/java9-failure

  1. 0001-unbundle-ASM.patch
    24/Dec/17 12:31 PM
    685 kB
    Nicola Mometto
  2. 0002-update-classfile-version-to-9.patch
    24/Dec/17 12:31 PM
    6 kB
    Nicola Mometto
  3. 0003-CLJ-2284.patch
    24/Dec/17 12:31 PM
    1.0 kB
    Nicola Mometto
  4. v2-0001-vendor-asm-to-sha-648c512e38e6cf58c09e22b324a9c99.patch
    17/Feb/18 3:57 PM
    1.83 MB
    Ghadi Shayban
  5. v2-0002-bump-javac-output-to-1.8-classfiles.patch
    17/Feb/18 3:57 PM
    4 kB
    Ghadi Shayban
  6. v2-0003-Compiler-genclass-emit-jdk8-classfiles.patch
    17/Feb/18 3:57 PM
    12 kB
    Ghadi Shayban
  7. v2-0004-CLJ-2284-enable-calls-to-static-interface-methods.patch
    17/Feb/18 3:57 PM
    2 kB
    Ghadi Shayban
  8. v2-0005-CLJ-2284-test-cases.patch
    17/Feb/18 3:57 PM
    2 kB
    Ghadi Shayban
  9. v3-0001-bump-javac-output-to-1.8-classfiles.patch
    10/Jun/18 9:21 PM
    4 kB
    Ghadi Shayban
  10. v3-0002-Compiler-genclass-emit-jdk8-classfiles.patch
    10/Jun/18 9:21 PM
    12 kB
    Ghadi Shayban
  11. v3-0003-CLJ-2284-enable-calls-to-static-interface-methods.patch
    10/Jun/18 9:21 PM
    2 kB
    Ghadi Shayban
  12. v3-0004-CLJ-2284-test-cases.patch
    10/Jun/18 9:21 PM
    2 kB
    Ghadi Shayban
  13. v3-0005-vendor-asm-to-sha-c72a86bd5f48a308537695213d3a23a.patch
    10/Jun/18 9:21 PM
    1.88 MB
    Ghadi Shayban

Activity

Hide
Ghadi Shayban added a comment -

Static or default interface method invocations are not supported when the bytecode emitted is < 1.8 bytecode (Clojure currently emits 1.6 bytecode). I'm mildly surprised this compiled.

Show
Ghadi Shayban added a comment - Static or default interface method invocations are not supported when the bytecode emitted is < 1.8 bytecode (Clojure currently emits 1.6 bytecode). I'm mildly surprised this compiled.
Hide
David Bürgin added a comment -

Not sure Ghadi’s assertion is correct? No new bytecodes were introduced for static and default interface methods. Or at least I have been using JDK 8 utilities like CharSequence.codePoints() and Comparator.reverseOrder() in Clojure for a long time. (And if this usage were not supported today that would seem like a critical issue.)

Show
David Bürgin added a comment - Not sure Ghadi’s assertion is correct? No new bytecodes were introduced for static and default interface methods. Or at least I have been using JDK 8 utilities like CharSequence.codePoints() and Comparator.reverseOrder() in Clojure for a long time. (And if this usage were not supported today that would seem like a critical issue.)
Hide
Zach Tellman added a comment -

Yeah, I'd expect default interface methods to just be filled in on classes which don't provide their own implementation, and empirically they don't seem to cause any issues, but I'm not an authority on JVM bytecode.

Show
Zach Tellman added a comment - Yeah, I'd expect default interface methods to just be filled in on classes which don't provide their own implementation, and empirically they don't seem to cause any issues, but I'm not an authority on JVM bytecode.
Hide
Nicola Mometto added a comment - - edited

while it is true that no bytecodes were introduced, the JVM spec mandates that invocations to static interface methods refer to an InterfaceMethodref entry in the constant pool rather than simply to a Methodref – as to why this worked in jvm 1.8 and fails with a verify error in jvm 9 , I can only assume the bytecode verifier became stricter since version 9

See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.3, describing the resolution rules for Methodrefs:

When resolving a method reference:

    If C is an interface, method resolution throws an IncompatibleClassChangeError.
Show
Nicola Mometto added a comment - - edited while it is true that no bytecodes were introduced, the JVM spec mandates that invocations to static interface methods refer to an InterfaceMethodref entry in the constant pool rather than simply to a Methodref – as to why this worked in jvm 1.8 and fails with a verify error in jvm 9 , I can only assume the bytecode verifier became stricter since version 9 See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.3, describing the resolution rules for Methodrefs:
When resolving a method reference:

    If C is an interface, method resolution throws an IncompatibleClassChangeError.
Hide
Nicola Mometto added a comment - - edited

Indeed here's the openjdk ticket tightening bytecode verification to adhere to the JVM spec: https://bugs.openjdk.java.net/browse/JDK-8145148
According to the last comment on that ticket, upgrading the ASM version provided with clojure to 5.1 should fix this

Show
Nicola Mometto added a comment - - edited Indeed here's the openjdk ticket tightening bytecode verification to adhere to the JVM spec: https://bugs.openjdk.java.net/browse/JDK-8145148 According to the last comment on that ticket, upgrading the ASM version provided with clojure to 5.1 should fix this
Hide
Nicola Mometto added a comment - - edited

The following 3 patches work to fix this problem, I've kept them split for now so that we can be evaluated separately, but in order to fix this all three are required.

1st patch simply updates the version of ASM that clojure uses to 6.0, removes the bundled ASM and adds ASM as a separate dependency. If this is not desirable we could just update the bundled version of ASM but it seems like now that clojure requires external deps, this is up for debate.

2nd patch updates the classfile version that clojure emits from 1_5 to 9 so that ASM can emit interface method refs, because of changes in bytecode verification since v1.5, several additional changes were needed:
1- it's not possible anymore to assign final fields from methods other than <clinit>, so it was necessary to mark the const__ fields as non final as they are assigned by init__n methods
2- stack map frames are not optional anymore so we need to tell ASM to compute them for us
3- a custom ClassWriter overriding getCommonSuperClass has been implemented, using DCL to load classes and short circuiting on classes that are being defined (as per that method's javadoc)

finally the third patch addresses this issue by emitting `invokeStatic` through the new `visitmethodInsn` arity that supports interface methods

Show
Nicola Mometto added a comment - - edited The following 3 patches work to fix this problem, I've kept them split for now so that we can be evaluated separately, but in order to fix this all three are required. 1st patch simply updates the version of ASM that clojure uses to 6.0, removes the bundled ASM and adds ASM as a separate dependency. If this is not desirable we could just update the bundled version of ASM but it seems like now that clojure requires external deps, this is up for debate. 2nd patch updates the classfile version that clojure emits from 1_5 to 9 so that ASM can emit interface method refs, because of changes in bytecode verification since v1.5, several additional changes were needed: 1- it's not possible anymore to assign final fields from methods other than <clinit>, so it was necessary to mark the const__ fields as non final as they are assigned by init__n methods 2- stack map frames are not optional anymore so we need to tell ASM to compute them for us 3- a custom ClassWriter overriding getCommonSuperClass has been implemented, using DCL to load classes and short circuiting on classes that are being defined (as per that method's javadoc) finally the third patch addresses this issue by emitting `invokeStatic` through the new `visitmethodInsn` arity that supports interface methods
Hide
Nicola Mometto added a comment -

Note that I'm not proposing the 3 patches as they are as a fix for this, given that bumping classfile version means discontinuing support for running clojure on java versions earlier than what's specified, which is undesiderable (I've bumped it to V9 but what's needed here should be just V1_8, but still).

That said, if we want to fix this, we definitely need the fixes I've put in those patches and we likely also want to conditionally emit different classfile versions (e.g. defaulting to 1_5 but bumping it to a higher version for specific classfiles when needed)

Show
Nicola Mometto added a comment - Note that I'm not proposing the 3 patches as they are as a fix for this, given that bumping classfile version means discontinuing support for running clojure on java versions earlier than what's specified, which is undesiderable (I've bumped it to V9 but what's needed here should be just V1_8, but still). That said, if we want to fix this, we definitely need the fixes I've put in those patches and we likely also want to conditionally emit different classfile versions (e.g. defaulting to 1_5 but bumping it to a higher version for specific classfiles when needed)
Hide
Vladimir Tsanev added a comment -

Nicola Mometto are sure you sure upgrade the V1_8 is needed to address interface static and/or default methods? CONSTANT_InterfaceMethodref is not new it was in the instruction set since forever (and in V1_6 particularly https://docs.oracle.com/javase/specs/jvms/se6/html/ClassFile.doc.html#42041).

Show
Vladimir Tsanev added a comment - Nicola Mometto are sure you sure upgrade the V1_8 is needed to address interface static and/or default methods? CONSTANT_InterfaceMethodref is not new it was in the instruction set since forever (and in V1_6 particularly https://docs.oracle.com/javase/specs/jvms/se6/html/ClassFile.doc.html#42041).
Hide
Nicola Mometto added a comment -

Vladimir Tsanev InterfaceMethodref is not new, but supporting `invokeStatic` on an InterfaceMethodref is only supported since V1_8

Show
Nicola Mometto added a comment - Vladimir Tsanev InterfaceMethodref is not new, but supporting `invokeStatic` on an InterfaceMethodref is only supported since V1_8
Hide
Ghadi Shayban added a comment -

Attached v2- patches. Please read patch-3 commit message carefully.

Show
Ghadi Shayban added a comment - Attached v2- patches. Please read patch-3 commit message carefully.
Hide
Ghadi Shayban added a comment -

I consider this a very high priority defect when running Clojure on Java 9. Some libraries (notably the new AWS SDK) are beginning to make use of static methods on interfaces.

The v2- patches enable JDK8 bytecode, which allows Clojure's bytecode to correctly call these methods without throwing an exception. This sheds compatibility for JDK 7. Android should no longer have problems with JDK8 bytecode.

ASM is now hosted on git, and the first patch's commit message contains a useful vendoring script.

As a side-effect loading Clojure is consistently about 5% faster across a few machines I tested (x86 & ARM). I did not test compilation speed.

Show
Ghadi Shayban added a comment - I consider this a very high priority defect when running Clojure on Java 9. Some libraries (notably the new AWS SDK) are beginning to make use of static methods on interfaces. The v2- patches enable JDK8 bytecode, which allows Clojure's bytecode to correctly call these methods without throwing an exception. This sheds compatibility for JDK 7. Android should no longer have problems with JDK8 bytecode. ASM is now hosted on git, and the first patch's commit message contains a useful vendoring script. As a side-effect loading Clojure is consistently about 5% faster across a few machines I tested (x86 & ARM). I did not test compilation speed.
Hide
Vladimir Tsanev added a comment -

asm 6.1 is released which does not have any new features (except java 10 bytecode version). It should be binary compatible, but have few runtime changes - probably worth checking if everything is ok against the new version. They also changed the code style - another good reason to unbundle asm since merging will become harder.

Show
Vladimir Tsanev added a comment - asm 6.1 is released which does not have any new features (except java 10 bytecode version). It should be binary compatible, but have few runtime changes - probably worth checking if everything is ok against the new version. They also changed the code style - another good reason to unbundle asm since merging will become harder.
Hide
Ghadi Shayban added a comment -

The patch in v2-0001 is already based on the ASM-6.1 branch but not the final release. Can bump it with the script in the commit comment.

Show
Ghadi Shayban added a comment - The patch in v2-0001 is already based on the ASM-6.1 branch but not the final release. Can bump it with the script in the commit comment.
Hide
Ghadi Shayban added a comment -

Workaround macro:

(defmacro interface-static-call
  [sym & argtypes]
  `(let [m# (.getMethod ~(symbol (namespace sym))
                        ~(name sym)
                        (into-array Class ~argtypes))]
     (fn [& args#]
       (.invoke m# nil (to-array args#)))))

(def client
  (interface-static-call S3Client/create))
Show
Ghadi Shayban added a comment - Workaround macro:
(defmacro interface-static-call
  [sym & argtypes]
  `(let [m# (.getMethod ~(symbol (namespace sym))
                        ~(name sym)
                        (into-array Class ~argtypes))]
     (fn [& args#]
       (.invoke m# nil (to-array args#)))))

(def client
  (interface-static-call S3Client/create))
Hide
Ghadi Shayban added a comment -

v3*.patch updates to ASM 6.2 final

Show
Ghadi Shayban added a comment - v3*.patch updates to ASM 6.2 final

People

Vote (9)
Watch (9)

Dates

  • Created:
    Updated: