Skip to content

Fix/initializer deadlocks #628

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jun 3, 2015
Merged

Fix/initializer deadlocks #628

merged 8 commits into from
Jun 3, 2015

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jun 1, 2015

Fixes all observed deadlocks by moving lambdas to static scope only if we must do so (because they are in a super constructor). Review by @DarkDimius

odersky added 2 commits June 1, 2015 10:05
@odersky
Revert "Avoid static initialization deadlock in run tests."
07ed3f1 
This reverts commit 8f90105.
@odersky
Avoid static initialization deadlock in run tests (2).
bb7d6dd 
See scala#624 (comment)
for a lengthy explanation.

We now solve the problem in LambdaLift. The formerly failing tests are
all reverted to theor original, failing, version.
@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2015

@DarkDimius Not sure whether we want to be more aggressive moving lambdas to static scope. But we should make sure that cannot cause a deadlock. The current behavior is safest.

@DarkDimius
Copy link
Contributor

@odersky can you please try this test:

class A(a: =>Object)
object foo extends A({
  object bar extends A({
     val s = (1 to 10).map(x => if (x % 2 == 0) bar else foo)
     s
    })
    val s = bar
    s
  });

In scalac, it would fail due to https://issues.scala-lang.org/browse/SI-7666 but this is valid code, broken by 3d240ad.

@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2015

@DarkDimius I am not super current on this one and am very time
constrained. Can you try to do a PR for me to review?

On Mon, Jun 1, 2015 at 3:07 PM, Dmitry Petrashko [email protected]
wrote:

@odersky https://github.com/odersky can you please try this test:

class A(a: =>Object)
object foo extends A({
object bar extends A(
{val s = (1 to 10).map(x => if (x % 2 == 0) bar else foo)
s
})
val s = bar
s
});

In scalac, it would fail due to
https://issues.scala-lang.org/browse/SI-7666 but this is valid code,
broken by 3d240ad
3d240ad
.

—
Reply to this email directly or view it on GitHub
#628 (comment).

Martin Odersky
EPFL

@smarter
Copy link
Member

smarter commented Jun 1, 2015

cafd71a needs to be reverted too for partest to succeed.

@DarkDimius
Copy link
Contributor

cafd71a shouldn't have been merged in the first place. The test was correct, and the tested behavior was correct.

@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2015

@DarkDimius Normally, I'd agree with you. But we had a failing build on master, that trumps everything else.

@smarter
Copy link
Member

smarter commented Jun 1, 2015

@DarkDimius: The tested behavior changed because the lambda creation changed: after #623 the lambda body was static so it didn't need any environment. Now the same lambda body isn't static anymore, and LambdaMetaFactory will create a method get$Lambda: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/87d4b7551d32/src/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#l281

@DarkDimius
Copy link
Contributor

@smarter I know how LMF works. What I was saying is that #623 included commits, that were incorrect in the first place. They were fixing failing tests by introducing new bugs, that allowed tests to pass.
Failure of t6260 was a clear manifestation of this, and instead of fixing the bug, in cafd71a the test was made to test for wrong behavior.

I believe that this is even worse than disabling the test, as we make them check for the behavior, that is obviously wrong.

@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2015

OK, let's fix all this ASAP. - Martin

On Mon, Jun 1, 2015 at 4:32 PM, Dmitry Petrashko [email protected]
wrote:

@smarter https://github.com/smarter I know how LMF works. What I was
saying is that #623 #623 included
commits, that were incorrect in the first place. They were fixing failing
tests by introducing new bugs, that allowed tests to pass.
Failure of t6260 was a clear manifestation of this, and instead of fixing
the bug, in cafd71a
cafd71a
the test was made to test for wrong behavior.

I believe that this is even worse than disabling the test, as we make them
check for the behavior, that is obviously wrong.

—
Reply to this email directly or view it on GitHub
#628 (comment).

Martin Odersky
EPFL

@DarkDimius
Copy link
Contributor

I'm already on it.

DarkDimius added 2 commits June 2, 2015 11:02
@DarkDimius
Revert "Update t6260-delambdafy.check to account for change in lambda…
f078735 
… generation"

This reverts commit cafd71a.
For the future refference: tests and checkfiles should be modified only after carefull thought.
Otherwise our tests will stop indicating the correct behaviour.
@DarkDimius
Revert "Fix ElimStaticThis#transformIdent"
4afa3ff 
This reverts commit 3d240ad.
This commit got in without succeding the review.
It broke what already was working(inner static objects),
and made impossible moving static methods from companion object to companion class.

Additionally, commenting or removing assertions is not the way to go, and should not pass review.

See discussion here: 3d240ad
@DarkDimius
Copy link
Contributor

It took quite some time to understand what happens here and why:
Lets have a look on this test tests/pos/lambdalift.scala:

class Super(x: Int)

class Sub extends Super({
  def foo3(x: Int) = {

    class C {
      def this(name: String) = this()

      def bam(y: Int): String => Int = {
        def baz = x + y
        z => baz * z.length
      }
    }

    val fun = new C("dummy").bam(1)
    fun("abc")

  }
  foo3(22)
})

Class C here gets a companion object C, which is owned by method foo3, and will undergo transformation of lazy vals as a local lazy val and all accesses to it will be replaced by a getter method. Than this method will be lifted out during LambdaLift, as it's defined inside super call and will get JavaStatic flag. By this will have a module class, which is a method, which has static definition, but which is not statically available. IE, both flags Module and JavaStatic will be set. This is the assertion failure that was silently disabled in #623.

@DarkDimius
Copy link
Contributor

I'm deep in process of grading Advanced compiler construction course, so I'll need to take some time to clear my mind and understand how this could be solved. Context switching between GC's written in C and Dotty kills my productivity entirely.

@odersky, I propose to revert 8f90105, 3d240ad and cafd71a, and move failing test to a separate file in pending. This way we'll have master that people can work on, until I fix the issue. I'll update the PR soon.

@odersky
Copy link
Contributor Author

odersky commented Jun 2, 2015

@DarkDimius Agreed on the reversal. 8f90105 is already reverted by 07ed3f1.

@DarkDimius DarkDimius force-pushed the fix/initializer-deadlocks branch from 045ce59 to 367c9d3 Compare June 2, 2015 13:43
@DarkDimius
Copy link
Contributor

@odersky bb7d6dd seems to have broken compilation of this labmda:
https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala#L857

It gets lifted to far, and cannot access this anymore, and fails in tree checker. Could you please have a look?
I'll come back to disabled test later.

@smarter
Copy link
Member

smarter commented Jun 2, 2015

Here's a minimized test case that fails:

class A {
  class B {
    def outer(): Unit = {
      def inner(): Int = 2

      val fi: Function0[Int] = () => inner()
    }
  }
}
[error] Exception in thread "main" java.lang.AssertionError: assertion failed: error while typing B.this, method $anonfun$1 is not contained in class A$B
[error]         at scala.Predef$.assert(Predef.scala:165)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedThis(TreeChecker.scala:238)
[error]         at dotty.tools.dotc.typer.Typer.typedUnnamed$1(Typer.scala:1044)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1083)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.ReTyper.typedSelect(ReTyper.scala:35)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedSelect(TreeChecker.scala:232)
[error]         at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:1023)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1081)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.ReTyper.typedSelect(ReTyper.scala:35)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedSelect(TreeChecker.scala:232)
[error]         at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:1023)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1081)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.Typer.typedExpr(Typer.scala:1130)
[error]         at dotty.tools.dotc.typer.Applications$$anonfun$realApply$1$1.apply(Applications.scala:533)
[error]         at dotty.tools.dotc.typer.Applications$$anonfun$realApply$1$1.apply(Applications.scala:529)
[error]         at dotty.tools.dotc.util.Stats$.track(Stats.scala:35)
[error]         at dotty.tools.dotc.typer.Applications$class.realApply$1(Applications.scala:529)
[error]         at dotty.tools.dotc.typer.Applications$class.typedApply(Applications.scala:595)
[error]         at dotty.tools.dotc.typer.Typer.typedApply(Typer.scala:58)
[error]         at dotty.tools.dotc.typer.Typer.typedUnnamed$1(Typer.scala:1043)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1083)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.Typer.typedExpr(Typer.scala:1130)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typedDefDef$1.apply(Typer.scala:897)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typedDefDef$1.apply(Typer.scala:890)
[error]         at dotty.tools.dotc.util.Stats$.track(Stats.scala:35)
[error]         at dotty.tools.dotc.typer.Typer.typedDefDef(Typer.scala:890)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.dotty$tools$dotc$transform$TreeChecker$Checker$$super$typedDefDef(TreeChecker.scala:265)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker$$anonfun$typedDefDef$1$$anonfun$apply$3.apply(TreeChecker.scala:265)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker$$anonfun$typedDefDef$1$$anonfun$apply$3.apply(TreeChecker.scala:265)
[error]         at dotty.tools.dotc.core.Decorators$ListDecorator$.foldRightBN$extension(Decorators.scala:105)
[error]         at dotty.tools.dotc.core.Decorators$ListDecorator$$anonfun$foldRightBN$extension$1.apply(Decorators.scala:106)
[error]         at dotty.tools.dotc.core.Decorators$ListDecorator$.foldRightBN$extension(Decorators.scala:105)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.withDefinedSyms(TreeChecker.scala:163)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker$$anonfun$withDefinedSymss$1.apply(TreeChecker.scala:166)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker$$anonfun$withDefinedSymss$1.apply(TreeChecker.scala:166)
[error]         at dotty.tools.dotc.core.Decorators$ListDecorator$.foldRightBN$extension(Decorators.scala:106)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.withDefinedSymss(TreeChecker.scala:166)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker$$anonfun$typedDefDef$1.apply(TreeChecker.scala:264)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker$$anonfun$typedDefDef$1.apply(TreeChecker.scala:264)
[error]         at dotty.tools.dotc.core.Decorators$ListDecorator$.foldRightBN$extension(Decorators.scala:105)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.withDefinedSyms(TreeChecker.scala:163)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedDefDef(TreeChecker.scala:263)
[error]         at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:1031)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1081)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.Typer.traverse$1(Typer.scala:1115)
[error]         at dotty.tools.dotc.typer.Typer.typedStats(Typer.scala:1126)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedStats(TreeChecker.scala:291)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typedClassDef$1.apply(Typer.scala:927)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typedClassDef$1.apply(Typer.scala:909)
[error]         at dotty.tools.dotc.util.Stats$.track(Stats.scala:35)
[error]         at dotty.tools.dotc.typer.Typer.typedClassDef(Typer.scala:909)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedClassDef(TreeChecker.scala:259)
[error]         at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:1034)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1081)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.Typer.traverse$1(Typer.scala:1115)
[error]         at dotty.tools.dotc.typer.Typer.typedStats(Typer.scala:1126)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedStats(TreeChecker.scala:291)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typedPackageDef$1.apply(Typer.scala:974)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typedPackageDef$1.apply(Typer.scala:965)
[error]         at dotty.tools.dotc.util.Stats$.track(Stats.scala:35)
[error]         at dotty.tools.dotc.typer.Typer.typedPackageDef(Typer.scala:965)
[error]         at dotty.tools.dotc.typer.Typer.typedUnnamed$1(Typer.scala:1071)
[error]         at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1083)
[error]         at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:93)
[error]         at dotty.tools.dotc.transform.TreeChecker$Checker.typedUnadapted(TreeChecker.scala:182)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1093)
[error]         at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1091)
[error]         at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:148)
[error]         at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:52)
[error]         at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1091)
[error]         at dotty.tools.dotc.typer.Typer.typedExpr(Typer.scala:1130)
[error]         at dotty.tools.dotc.transform.TreeChecker.check(TreeChecker.scala:116)
[error]         at dotty.tools.dotc.transform.TreeChecker.run(TreeChecker.scala:94)
[error]         at dotty.tools.dotc.core.Phases$Phase$$anonfun$runOn$1.apply(Phases.scala:270)
[error]         at dotty.tools.dotc.core.Phases$Phase$$anonfun$runOn$1.apply(Phases.scala:268)
[error]         at scala.collection.immutable.List.map(List.scala:273)
[error]         at dotty.tools.dotc.core.Phases$Phase$class.runOn(Phases.scala:268)
[error]         at dotty.tools.dotc.transform.TreeChecker.runOn(TreeChecker.scala:38)
[error]         at dotty.tools.dotc.Run$$anonfun$compileUnits$1$$anonfun$apply$mcV$sp$1.apply(Run.scala:59)
[error]         at dotty.tools.dotc.Run$$anonfun$compileUnits$1$$anonfun$apply$mcV$sp$1.apply(Run.scala:56)
[error]         at scala.collection.IndexedSeqOptimized$class.foreach(IndexedSeqOptimized.scala:33)
[error]         at scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:186)
[error]         at dotty.tools.dotc.Run$$anonfun$compileUnits$1.apply$mcV$sp(Run.scala:56)
[error]         at dotty.tools.dotc.Run$$anonfun$compileUnits$1.apply(Run.scala:52)
[error]         at dotty.tools.dotc.Run$$anonfun$compileUnits$1.apply(Run.scala:52)
[error]         at dotty.tools.dotc.util.Stats$.monitorHeartBeat(Stats.scala:68)
[error]         at dotty.tools.dotc.Run.compileUnits(Run.scala:52)
[error]         at dotty.tools.dotc.Run.compileSources(Run.scala:49)
[error]         at dotty.tools.dotc.Run.compile(Run.scala:33)
[error]         at dotty.tools.dotc.Driver.doCompile(Driver.scala:20)
[error]         at dotty.tools.dotc.Main$.doCompile(Main.scala:26)
[error]         at dotty.tools.dotc.Driver.process(Driver.scala:31)
[error]         at dotty.tools.dotc.Driver.main(Driver.scala:40)
[error]         at dotty.tools.dotc.Main.main(Main.scala)

odersky added 3 commits June 3, 2015 18:10
@odersky
Narrow liftedOwner also for static objects
5cb4ecb 
Lambdas should stay inside static objects if they reference to them with
a This or Ident. Fixes test case `llist.scala` but demonstrates another problem.
@odersky
Compute outer.path in lambdaLift at correct phase.
6c8a242 
LambdaLift needs to compute outer.path at the phase in which
the results are constructed, i.e. phase lambdaLift.next. Or else
we get an error in outer.path for lost fo files, including pos/Fileish.scala
as a minimized test case. Previously outer as computed at phase lambdaLift.
The reason for this is that lambdaLift name mangles inner classes, which
causes outer acessors to be not found. We now correct for the problem
in outer.path itself, by calling outerAccessor only at a safe phase.
@odersky
Test case for problems with lambda lifting.
bcf70e2
@odersky
Copy link
Contributor Author

odersky commented Jun 3, 2015

@DarkDimius Please review.

@DarkDimius
Copy link
Contributor

LGTM

DarkDimius added a commit that referenced this pull request Jun 3, 2015
@DarkDimius
Merge pull request #628 from dotty-staging/fix/initializer-deadlocks
c0770ed 
Fix/initializer deadlocks
@DarkDimius DarkDimius merged commit c0770ed into scala:master Jun 3, 2015
@odersky odersky deleted the fix/initializer-deadlocks branch June 8, 2015 10:42
@DarkDimius DarkDimius mentioned this pull request Apr 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@odersky @DarkDimius @smarter @dottybot