make module init compliant with the jvm spec

[retronym]

Details: scala/scala3#1441

Proposed fix: scala/scala@2.12.x...retronym:topic/module-init

[retronym]

Worth considering for 2.12.0 IMO. It will only blow up on Java 9, which we don't claim to support yet, but it would be good to at least have support for running code that we've compiled against JDK8 in JDK9.

[adriaanm]

Agreed.

[DarkDimius]

@retronym see comment here scala/scala3#1441 (comment) that shows a counterexample to your solution and a possible fix.

[retronym]

@DarkDimius Good catch. That is very subtle: we currently set the field after the module's super constructors are called but before the rest of the module's own constructor is called.

Perhaps our safest bet for 2.12.x is to make the static MODULE$ field non-final in bytecode. We're already subverting its finality by assigning is in the <init> rather than <clinit>.

In the future, should consider changing the putfield to a compare-and-set that enforces that the field is only written once (ensureAccessible(O.getClass).newInstance() should not be able to overwrite the field as it can today.

[adriaanm]

Since this only blows up on bytecode version 53, which we won't generate for a while, I'd vote for doing nothing for 2.12. Removing the final modifier has an impact on the memory model, right?

[retronym]

https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.5

The clinit method is called within an initialisation lock that takes care of publishing the field write to other threads

[retronym]

But I wasn't aware of version 53, so agree this could be pushed back.

https://bugs.openjdk.java.net/browse/JDK-8148785

[adriaanm]

I'm a little uneasy about punting on this because I couldn't find a public commitment to not close this loophole in old classfiles, but I'm more worried about breaking our initialization semantics on current Java.

It's still not clear to me how doing a putstatic to a non-final field in the <init> method ensures safe publications. I see that the <clinit> method establishes a happens-before relation, but is the same true for the regular constructor, as we can't use the clinit method due to cycles?

[DarkDimius]

It's still not clear to me how doing a putstatic to a non-final field in the <init> method ensures safe publications.

In our current state clinit calls init that assigns a MODULE$ field.
clinit is run inside a synchronized block that ensures safe publication.

If you would call a module-class constructor using reflection, this would not be a safe publication, but this should be simply prohibited, ie constructor should check that current value of MODULE$ field is null.

[adriaanm]

Ah, thanks for explaining!

[adriaanm]

scala/scala#5322