Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixin fields with trait setters shouldn't be JVM final #7028

Merged
merged 3 commits into from
Aug 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/compiler/scala/tools/nsc/transform/Constructors.scala
Original file line number Diff line number Diff line change
Expand Up @@ -756,11 +756,16 @@ abstract class Constructors extends Statics with Transform with TypingTransforme
if (isDelayedInitSubclass && remainingConstrStats.nonEmpty) delayedInitDefsAndConstrStats(defs, remainingConstrStats)
else (Nil, remainingConstrStats)

val fence = if (clazz.primaryConstructor.hasAttachment[ConstructorNeedsFence.type]) {
val tree = localTyper.typedPos(clazz.primaryConstructor.pos)(gen.mkRuntimeCall(nme.releaseFence, Nil))
tree :: Nil
} else Nil

// Assemble final constructor
val primaryConstructor = deriveDefDef(primaryConstr)(_ => {
treeCopy.Block(
primaryConstrBody,
paramInits ::: constructorPrefix ::: uptoSuperStats ::: guardSpecializedInitializer(remainingConstrStatsDelayedInit),
paramInits ::: constructorPrefix ::: uptoSuperStats ::: guardSpecializedInitializer(remainingConstrStatsDelayedInit) ::: fence,
primaryConstrBody.expr)
})

Expand Down
21 changes: 16 additions & 5 deletions src/compiler/scala/tools/nsc/transform/Fields.scala
Original file line number Diff line number Diff line change
Expand Up @@ -422,12 +422,12 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
// println(s"expanded modules for $clazz: $expandedModules")

// afterOwnPhase, so traits receive trait setters for vals (needs to be at finest grain to avoid looping)
val synthInSubclass =
val synthInSubclass: List[Symbol] =
clazz.mixinClasses.flatMap(mixin => afterOwnPhase(mixin.info).decls.toList.filter(accessorImplementedInSubclass))

// mixin field accessors --
// invariant: (accessorsMaybeNeedingImpl, mixedInAccessorAndFields).zipped.forall(case (acc, clone :: _) => `clone` is clone of `acc` case _ => true)
val mixedInAccessorAndFields = synthInSubclass.map{ member =>
val mixedInAccessorAndFields: List[List[Symbol]] = synthInSubclass.map{ member =>
def cloneAccessor() = {
val clonedAccessor = (member cloneSymbol clazz) setPos clazz.pos
setMixedinAccessorFlags(member, clonedAccessor)
Expand Down Expand Up @@ -659,9 +659,20 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor
// trait val/var setter mixed into class
else fieldAccess(setter) match {
case NoSymbol => EmptyTree
case fieldSel => afterOwnPhase { // the assign only type checks after our phase (assignment to val)
mkAccessor(setter)(Assign(Select(This(clazz), fieldSel), castHack(Ident(setter.firstParam), fieldSel.info)))
}
case fieldSel =>
if (!fieldSel.hasFlag(MUTABLE)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a comment: If the field is mutable, it won't be final, so we can write to it in a setter. If it's not, we still need to initialize it, and make sure it's safely published. Since initialization is performed (lexically) outside of the constructor (in the trait setter), we have to make the field mutable starting with classfile format 53 (it was never allowed, but the verifier enforces this now).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated with this comment

// If the field is mutable, it won't be final, so we can write to it in a setter.
// If it's not, we still need to initialize it, and make sure it's safely published.
// Since initialization is performed (lexically) outside of the constructor (in the trait setter),
// we have to make the field mutable starting with classfile format 53
// (it was never allowed, but the verifier enforces this now).
fieldSel.setFlag(MUTABLE)
fieldSel.owner.primaryConstructor.updateAttachment(ConstructorNeedsFence)
}

afterOwnPhase { // the assign only type checks after our phase (assignment to val)
mkAccessor(setter)(Assign(Select(This(clazz), fieldSel), castHack(Ident(setter.firstParam), fieldSel.info)))
}
}

def moduleAccessorBody(module: Symbol): Tree =
Expand Down
5 changes: 5 additions & 0 deletions src/library/scala/runtime/ScalaRunTime.scala
Original file line number Diff line number Diff line change
Expand Up @@ -268,4 +268,9 @@ object ScalaRunTime {
def wrapShortArray(xs: Array[Short]): ArraySeq[Short] = if (xs ne null) new ArraySeq.ofShort(xs) else null
def wrapBooleanArray(xs: Array[Boolean]): ArraySeq[Boolean] = if (xs ne null) new ArraySeq.ofBoolean(xs) else null
def wrapUnitArray(xs: Array[Unit]): ArraySeq[Unit] = if (xs ne null) new ArraySeq.ofUnit(xs) else null

/**
* Forwards to `VarHandle.releaseFence` or `sun.misc.Unsafe.storeFence` on JDK9+ or JDK8 respectively.
*/
@inline def releaseFence(): Unit = VM.RELEASE_FENCE.invoke()
}
46 changes: 46 additions & 0 deletions src/library/scala/runtime/VM.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package scala.runtime;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.lang.reflect.Field;

public class VM {
public static final MethodHandle RELEASE_FENCE;


static {
RELEASE_FENCE = mkHandle();
}

private static MethodHandle mkHandle() {
MethodHandles.Lookup lookup = MethodHandles.lookup();
try {
return lookup.findStatic(Class.forName("java.lang.invoke.VarHandle"), "releaseFence", MethodType.methodType(Void.TYPE));
} catch (ClassNotFoundException e) {
try {
Class<?> unsafeClass = Class.forName("sun.misc.Unsafe");
return lookup.findVirtual(unsafeClass, "storeFence", MethodType.methodType(void.class)).bindTo(findUnsafe(unsafeClass));
} catch (NoSuchMethodException | ClassNotFoundException | IllegalAccessException e1) {
ExceptionInInitializerError error = new ExceptionInInitializerError(e1);
error.addSuppressed(e);
throw error;
}
} catch (NoSuchMethodException | IllegalAccessException e) {
throw new ExceptionInInitializerError(e);
}
}

private static Object findUnsafe(Class<?> unsafeClass) throws IllegalAccessException {
Object found = null;
for (Field field : unsafeClass.getDeclaredFields()) {
if (field.getType() == unsafeClass) {
field.setAccessible(true);
found = field.get(null);
break;
}
}
if (found == null) throw new IllegalStateException("No instance of Unsafe found");
return found;
}
}
2 changes: 2 additions & 0 deletions src/reflect/scala/reflect/internal/StdAttachments.scala
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,6 @@ trait StdAttachments {
case object KnownDirectSubclassesCalled extends PlainAttachment

class QualTypeSymAttachment(val sym: Symbol)

case object ConstructorNeedsFence extends PlainAttachment
}
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/internal/StdNames.scala
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,7 @@ trait StdNames {
val productPrefix: NameType = "productPrefix"
val raw_ : NameType = "raw"
val readResolve: NameType = "readResolve"
val releaseFence: NameType = "releaseFence"
val reify : NameType = "reify"
val reificationSupport : NameType = "reificationSupport"
val rootMirror : NameType = "rootMirror"
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse =>
this.UseInvokeSpecial
this.TypeParamVarargsAttachment
this.KnownDirectSubclassesCalled
this.ConstructorNeedsFence
this.noPrint
this.typeDebug
// inaccessible: this.posAssigner
Expand Down
23 changes: 18 additions & 5 deletions test/files/run/lazy-locals-2.scala
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,8 @@ object Test {
lzyComputeMethods == expComputeMethods,
s"wrong lzycompute methods. expected:\n$expComputeMethods\nfound:\n$lzyComputeMethods")

val fields = c.getClass.getDeclaredFields.toList.sortBy(_.getName).map(_.toString)
val expFields = List(
val fields: List[String] = c.getClass.getDeclaredFields.toList.sortBy(_.getName).map(_.toString)
val expFields = List[String](
"private volatile byte C.bitmap$0",
"private int C.lvl1",
"private java.lang.String C.lvl2",
Expand All @@ -305,9 +305,22 @@ object Test {
d.run()

val dFields = d.getClass.getDeclaredFields.toList.sortBy(_.getName).map(_.toString)
assert(
dFields == expFields.map(_.replaceAll(" C.", " D.")),
s"wrong fields. expected:\n$expFields\nfound:\n$fields")
val expDFields = List[String](
"private volatile byte D.bitmap$0",
"private int D.lvl1",
"private java.lang.String D.lvl2",
"private scala.runtime.BoxedUnit D.lvl3",
"private int D.t1",
"private java.lang.String D.t2",
"private scala.runtime.BoxedUnit D.t3",
"private int D.vl1",
"private java.lang.String D.vl2",
"private scala.runtime.BoxedUnit D.vl3",
"private int D.vr1",
"private java.lang.String D.vr2",
"private scala.runtime.BoxedUnit D.vr3")
assert(dFields == expDFields,
s"wrong fields. expected:\n$expDFields\nfound:\n$dFields")


val d1 = new D1
Expand Down
4 changes: 2 additions & 2 deletions test/files/run/t10075b.check
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@
@RetainedAnnotation() public int TMix.lzyValGetterAnnotation()
private int TMix.lzyValGetterAnnotation$lzycompute()
@RetainedAnnotation() public int TMix.method()
@RetainedAnnotation() private final int TMix.valFieldAnnotation
@RetainedAnnotation() private int TMix.valFieldAnnotation
public int TMix.valFieldAnnotation()
private final int TMix.valGetterAnnotation
private int TMix.valGetterAnnotation
@RetainedAnnotation() public int TMix.valGetterAnnotation()
@RetainedAnnotation() private int TMix.varFieldAnnotation
public int TMix.varFieldAnnotation()
Expand Down
2 changes: 2 additions & 0 deletions test/jcstress/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
*.gz
results
11 changes: 11 additions & 0 deletions test/jcstress/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
enablePlugins(JCStressPlugin)

scalaVersion := "2.13.0-pre-SNAPSHOT"

scalaHome := Some(baseDirectory.value / "../../build/pack")
version in Jcstress := "0.4"
resolvers in Global += Resolver.mavenLocal
crossPaths := false

// workaround https://github.com/ktoso/sbt-jcstress/issues/12
internalDependencyClasspath in Test := Nil
1 change: 1 addition & 0 deletions test/jcstress/project/build.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
sbt.version=1.1.5
1 change: 1 addition & 0 deletions test/jcstress/project/plugins.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
addSbtPlugin("pl.project13.scala" % "sbt-jcstress" % "0.2.0")
86 changes: 86 additions & 0 deletions test/jcstress/src/test/scala/example/APISample_01_Simple.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package example

/*
* Copyright (c) 2016, Red Hat Inc.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

import java.util.concurrent.atomic._

import org.openjdk.jcstress.annotations.Outcome.Outcomes
import org.openjdk.jcstress.annotations._
import org.openjdk.jcstress.infra.results.II_Result

/*
This is our first concurrency test. It is deliberately simplistic to show
testing approaches, introduce JCStress APIs, etc.
Suppose we want to see if the field increment is atomic. We can make test
with two actors, both actors incrementing the field and recording what
value they observed into the result object. As JCStress runs, it will
invoke these methods on the objects holding the field once per each actor
and instance, and record what results are coming from there.
Done enough times, we will get the history of observed results, and that
would tell us something about the concurrent behavior. For example, running
this test would yield:
[OK] o.o.j.t.JCStressSample_01_Simple
(JVM args: [-server])
Observed state Occurrences Expectation Interpretation
1, 1 54,734,140 ACCEPTABLE Both threads came up with the same value: atomicity failure.
1, 2 47,037,891 ACCEPTABLE actor1 incremented, then actor2.
2, 1 53,204,629 ACCEPTABLE actor2 incremented, then actor1.
How to run this test:
$ java -jar jcstress-samples/target/jcstress.jar -t JCStress_APISample_01_Simple
*/

// Mark the class as JCStress test.
@JCStressTest
@Description("Simple test, checking AtomicInteger")
@Outcomes(Array(
new Outcome(
id = Array("1, 1"),
expect = Expect.ACCEPTABLE_INTERESTING,
desc = "Both actors came up with the same value: atomicity failure."),
new Outcome(
id = Array("1, 2"),
expect = Expect.ACCEPTABLE,
desc = "actor1 incremented, then actor2."),
new Outcome(id = Array("2, 1"),
expect = Expect.ACCEPTABLE, desc = "actor2 incremented, then actor1.")
)
)
@State
class APISample_01_Simple {

var v = new AtomicInteger(0)

@Actor
def actor1(r: II_Result): Unit = {
r.r1 = v.incrementAndGet() // record result from actor1 to field r1
}

@Actor
def actor2(r: II_Result): Unit = {
r.r2 = v.incrementAndGet() // record result from actor1 to field r1
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package scala.collection.immutable

import org.openjdk.jcstress.annotations._
import org.openjdk.jcstress.annotations.Outcome.Outcomes
import org.openjdk.jcstress.infra.results._

@JCStressTest
@Outcomes(Array(
new Outcome(id = Array("-1, 0"), expect = Expect.ACCEPTABLE, desc = "Read before write"),
new Outcome(id = Array("16, 0"), expect = Expect.ACCEPTABLE, desc = "Read after all writes")
))
@State
class ListLikeStressTest {

var v: ListLike = _

@Actor
def actor1(r: II_Result): Unit = {
val list = new ListLike("a")
var l = list
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = new ListLike("a")
l = l.tail
l.tail = NilLike
//java.lang.invoke.VarHandle.releaseFence()
v = list

}

@Actor
def actor2(r: II_Result): Unit = {
var l = v
if (l eq null) {
r.r1 = -1
r.r2 = 0
return
}
var len = 0
var nulls = 0
while (l ne NilLike) {
if (l eq null) {r.r1 = len; r.r2 = nulls + 1; return}
if (l.head eq null) nulls += 1
assert(l ne l.tail)
l = l.tail
len += 1
}
r.r1 = len
r.r2 = nulls

}

}

class ListLike(var head: AnyRef) {
var tail: ListLike = _
}

object NilLike extends ListLike(null)
Loading