Clojure

Some static initialisers still run at compile time if used in type hints

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.9
  • Component/s: None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

Approach: Don't cause class to load merely from being in a type hint.

Patch: clj-1714-4.patch

This patch has been used in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).

Prescreened: Alex Miller

  1. CLJ-1714.patch
    22/Apr/15 2:37 PM
    1 kB
    Adam Clements
  2. CLJ-1714-v2.patch
    12/Aug/15 9:12 AM
    3 kB
    Adam Clements
  3. CLJ-1714-v3.patch
    28/Sep/16 2:07 PM
    3 kB
    Michael Blume
  4. clj-1714-4.patch
    28/Sep/16 2:40 PM
    4 kB
    Alex Miller

Activity

Hide
Alex Miller added a comment -

I think this might have been logged already but I'm not sure.

Show
Alex Miller added a comment - I think this might have been logged already but I'm not sure.
Hide
Michael Blume added a comment -

Patch won't apply to master for me

Show
Michael Blume added a comment - Patch won't apply to master for me
Hide
Adam Clements added a comment -

Really sorry, don't know what happened there. I checked out a fresh copy of the repo and re-applied the changes, deleted the old patch as it was garbage. Try the new one, timestamped 2:37pm

Show
Adam Clements added a comment - Really sorry, don't know what happened there. I checked out a fresh copy of the repo and re-applied the changes, deleted the old patch as it was garbage. Try the new one, timestamped 2:37pm
Hide
Stuart Halloway added a comment -

Please add an example of the problem, and if possible a failing test.

Show
Stuart Halloway added a comment - Please add an example of the problem, and if possible a failing test.
Hide
Alex Miller added a comment -

Reset to "Open" as moving from Triaged->Incomplete is not valid in our current workflow.

Show
Alex Miller added a comment - Reset to "Open" as moving from Triaged->Incomplete is not valid in our current workflow.
Hide
Adam Clements added a comment -

Example problem:
AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time.

I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).

Show
Adam Clements added a comment - Example problem: AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint. In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time. This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect). This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time. I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).
Hide
Stuart Halloway added a comment -

Hi Adam,

Thanks for the quick response. I think checking for static initializers being run is OK for a test.

Show
Stuart Halloway added a comment - Hi Adam, Thanks for the quick response. I think checking for static initializers being run is OK for a test.
Hide
Adam Clements added a comment -

Added failing tests which now pass

Show
Adam Clements added a comment - Added failing tests which now pass
Hide
Michael Blume added a comment -

Updated patch to apply to master

Show
Michael Blume added a comment - Updated patch to apply to master
Hide
Alex Miller added a comment -

Added new patch that is semantically identical, just easier to read (with formatted using -U8). Attribution retained.

Show
Alex Miller added a comment - Added new patch that is semantically identical, just easier to read (with formatted using -U8). Attribution retained.

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: