The locking macro fails bytecode verification on Graal native-image and ART runtime

Description

Android ART runs compile time verification on bytecode and fails on any usage of the locking macro. The error looks like that seen in (in that case clojure.core.server/stop-server calls the locking macro):

Graal fails with this error:

Cause: From discussion on an Android issue (https://code.google.com/p/android/issues/detail?id=80823), it seems this is due to more strictly enforcing the "structural locking" provisions in the JVM spec (https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10). Examination of the bytecode as compared to a java synchronized block shows up a number of differences: https://gist.github.com/AdamClements/2ae6c4919964b71eb470

It appears that the mixture of monitor-enter, monitor-exit, and the try/finally block in `locking` create paths that ART is flagging as not having balanced monitorenter/monitorexit bytecode. Particularly, monitorenter and monitorexit themselves can throw (on a null locking object). The Java bytecode does some tricky exception table handling to cover these cases which (afaict) is not possible to do without modifying the Clojure compiler.

Approaches:

  1. Fix Compiler's use of monitorenter/monitorexit to properly address the concerns above. This is difficult to do cleanly because the changes to monitorexit are contextual to whether it's in a try/finally etc. Patch CLJ-1472-reentrant-finally2.patch adds a new special compiler metadata hint to trigger the correct behavior, see details.

  2. Pass the body of `locking` into a synchronized method (basically bypass monitorenter/monitorexit entirely). clj-1472-4.patch takes this approach.

  3. Make `locking` into a special form and have the Compiler basically emit the same synchronized block as the prior alternative. No patch has been built for this as it's probably less favorable than the prior alternatives.

Perf testing:

Perf comparison, both w/ and w/o locking elision just to make sure we're not affecting that for Clojure 1.10.1, and with both leading patches:

There is a small decrease in performance for both patches, but given how `locking` is used and its frequency, this does not seem like an important difference.

Using code:

Screener feedback: (Alex Miller). Approach #1 (compiler hint+modification), is fairly complicated and adds a hint that is used in exactly one place (inside locking), so seems like a long way to go for something that can't really fix the underlying lack of context. Approach #2 (clj-1472-4.patch, locking body) is straightforward, has at this point been widely tested, and does not seem to introduce a performance issue. Recommend #2 (clj-1472-4.patch).

Patch: clj-1472-4.patch

Environment

Android ART runtime

Assignee

Unassigned

Reporter

AdamClements

Labels

Approval

Vetted

Patch

Code

Affects versions

Priority

Critical
Configure