SI-7974 Fix broken 'Symbol-handling code in CleanUp #3149

soc · 2013-11-17T02:59:09Z

The transformation did never happen because the pattern
failed to match. Why did the pattern fail to match? Because
Symbol_apply turned into an overloaded Symbol after erasure.

soc · 2013-11-18T01:10:23Z

@retronym

determine if the original code ever worked. If not, when did it start failing? >> It worked in 2.8.2 and 2.9.3, it doesn't work anymore in 2.10.3.
I think it is broken for {???; Symbol}.apply("foo") ... >> doesn't the use of fullName prevent that?
needs to be discussed ... >> I'll ask on the mailing list.
deeper understanding of the two symbols for apply you encounter. ... >> The culprit seems to be https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Definitions.scala#L1223

soc · 2013-11-18T01:23:46Z

Maybe @xeno-by has an idea about https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Definitions.scala#L1223 ... git blame says that the comments are from him.

xeno-by · 2013-11-18T09:53:17Z

Yeah, this behavior has been puzzling me as well, but I haven't been able
to find out why it happens.

On 18 November 2013 02:23, soc notifications@github.com wrote:

Maybe @xeno-by https://github.com/xeno-by has an idea about
https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Definitions.scala#L1223... git blame says that the comments are from him.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3149#issuecomment-28669976
.

retronym · 2013-11-19T07:09:47Z

Symbol_apply calls SymbolModule.info.nonPrivateMember(TermName("apply")), which returns an overloaded symbol containing the alternatives Symbol#apply and UniquenessCache#apply.

scala> :power

scala> enteringErasure(getMemberMethod(SymbolModule, nme.apply).alternatives.map(SymbolModule.info memberType _))
res22: List[$r.intp.global.Type] = List((name: String)Symbol)

scala> exitingErasure(getMemberMethod(SymbolModule, nme.apply).alternatives.map(SymbolModule.info memberType _))
res23: List[$r.intp.global.Type] = List((name: String)Symbol, (name: Object)Object)

scala> exitingErasure(Symbol_apply).alternatives.map(_.debugLocationString)
res9: List[String] = List(override method apply in object Symbol, method apply in class UniquenessCache)

It would seem that any call to findMember exitingErasure or later is a possible bug. You could wrap that call to the most localized fix for this PR is to wrap the call to Symbol_apply in exitingTyper(...).

You could add a followup commit with a more wide-spread fix

Change:

def getMemberIfDefined(owner: Symbol, name: Name): Symbol =
      owner.info.nonPrivateMember(name)

To:

def getMemberIfDefined(owner: Symbol, name: Name): Symbol =
      // findMember considered harmful after erasure; e.g. 
      // 
      // scala> exitingErasure(Symbol_apply).isOverloaded
      // res27: Boolean = true
      //
      enteringPhaseNotLaterThan(currentRun.erasurePhase)(
        owner.info.nonPrivateMember(name)
      )

And remove the localised fix, and @xeno-by's comment workaround in getMember

Back to the motivating topic of this PR (static field initializers not going into <clinit>), I notice that the caches for structural types are also static fields, and do indeed appear to be emitted in the right way:

% cat sandbox/symbol.scala && qbin/scalac sandbox/symbol.scala && javap -p -v C
class C {
  def foo = 0

  def bar: Any = (this: { def foo: Any}).foo
}
warning: there were 1 feature warning(s); re-run with -feature for details
one warning found

Compiled from "symbol.scala"
public class C extends java.lang.Object
...
{
private static java.lang.Class[] reflParams$Cache1;

private static volatile java.lang.ref.SoftReference reflPoly$Cache1;

public static {};
  Code:
   Stack=4, Locals=0, Args_size=0
   0:   iconst_0
   1:   anewarray   #12; //class java/lang/Class
   4:   putstatic   #16; //Field reflParams$Cache1:[Ljava/lang/Class;
   7:   new #18; //class java/lang/ref/SoftReference
   10:  dup
   11:  new #20; //class scala/runtime/EmptyMethodCache
   14:  dup
   15:  invokespecial   #23; //Method scala/runtime/EmptyMethodCache."<init>":()V
   18:  invokespecial   #26; //Method java/lang/ref/SoftReference."<init>":(Ljava/lang/Object;)V
   21:  putstatic   #30; //Field reflPoly$Cache1:Ljava/lang/ref/SoftReference;
   24:  return
  LineNumberTable:
   line 4: 0

...

}

soc · 2013-11-19T13:10:11Z

@retronym Wow, thanks a lot for debugging this! Just trying to understand this correctly ... why do we gain that second member after erasure in the first place?

soc · 2013-11-19T13:14:37Z

Back to the motivating topic of this PR (static field initializers not going into ), I notice that the caches for structural types are also static fields, and do indeed appear to be emitted in the right way: ...

Yes, I think the pattern is that we have a few specialized locations were work is done to make statics work correctly, but we are missing the bigger picture. I'm already looking into https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/transform/Constructors.scala#L634 were I think the appropriate general check is missing.

soc · 2013-11-19T13:49:17Z

@retronym

enteringPhaseNotLaterThan(currentRun.erasurePhase)(...

Won't this cause problems because currentRun lives on global and global is in the compiler jar, not the reflection jar?

Is something like https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/util/TraceSymbolActivity.scala#L90 a better way?

retronym · 2013-11-19T14:23:41Z

You are right. We can still get to it with the method used in src/reflect/scala/reflect/internal/util/TraceSymbolActivity.scala; we should refactor the private method runBeforeErasure out to be a member of SymbolTable.

soc · 2013-11-19T22:09:00Z

Looks like 144e416 is failing due to whitespace differences ... is there some magic incantation for that, or should I just call split(" ") on everything?

soc · 2013-11-20T00:51:09Z

sigh ... JavapTest is pretty terrible to debug ... I think I finally figured it out.

som-snytt · 2013-11-20T03:54:44Z

I was going to say something like, Wow, someone is actually using JavapTest, but I didn't want to discourage you.

Was it the bejahen thing that grabbed you?

soc · 2013-11-20T03:57:03Z

No, it just feels like the implementation does extra work in ca 10000 se of an failure to not actually tell anyone what the problem was ... but I figured it out now. :-)

retronym · 2013-11-20T07:15:15Z

You should actually use a bytecodetest to decouple from the rep.

On Wednesday, November 20, 2013, soc wrote:

No, it just feels like the implementation does extra work in case of an
failure to not actually tell anyone what the problem was ... but I figured
it out now. :-)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3149#issuecomment-28861963
.

adriaanm · 2013-11-20T07:19:55Z

PLS REBUILD ALL

typesafehub/sbt-builds-for-ide@08cfc9d should fix ide build breakage (in my local testing, sbt 0.13 was the default)

scala-jenkins · 2013-11-20T07:27:20Z

(kitty-note-to-self: ignore 28868185)
🐱 Roger! Rebuilding pr-scala for 1b804786, 37109b4f. 🚨

soc · 2013-11-21T14:05:26Z

PLS REBUILD/pr-scala@1b80478

scala-jenkins · 2013-11-21T14:07:27Z

(kitty-note-to-self: ignore 28986460)
🐱 Roger! Rebuilding pr-scala for 1b804786. 🚨

soc · 2013-11-21T14:50:47Z

PLS REBUILD/pr-scala@37109b4

soc · 2013-11-21T14:51:38Z

If this fails because javap decided to emit ”extends java.lang.Object” in javap 6 but stopped doing that in javap 7, I'll pull my hair out ...

scala-jenkins · 2013-11-21T14:57:27Z

(kitty-note-to-self: ignore 28990076)
🐱 Roger! Rebuilding pr-scala for 37109b4f. 🚨

soc · 2013-11-22T14:50:47Z

PLS REBUILD ALL

scala-jenkins · 2013-11-22T14:57:26Z

(kitty-note-to-self: ignore 29077590)
🐱 Roger! Rebuilding pr-scala for 1b804786, 3f05d2f8. 🚨

soc · 2013-11-22T23:29:39Z

Wow, this must have been my most painful, nerve-wrecking test case ever.
@retronym Is this good to go?

adriaanm

Thanks for persevering, @soc!

retronym · 2013-11-24T07:40:05Z

We should not involve the REPL and :javap in this test case. It should be written as ByteCodeTest. Sorry my earlier comment didn't make the details clearer, I sent it from my phone.

Here's an example that used ASM to output the decompiled bytecode and compares it against a checkfile:

retronym@178c37bdded#diff-9821ea5ae9849e5bd4c52285ec2e56f5R5

retronym · 2013-11-24T07:49:55Z

src/reflect/scala/reflect/internal/Definitions.scala

        case x: TermSymbol => x
        case _             => fatalMissingSymbol(owner, name, "method")
      }
    }

+    private lazy val erasurePhase = findPhaseWithName("erasure")


This might be brittle in a compiler that doesn't have an "erasure" phase. (Maybe ScalaJS?)

How about?

final def findPhaseBeforeErasure: Phase = { var ph = phase while (ph != NoPhase && ph.erasedTypes) { ph = ph.prev } if (ph eq NoPhase) phase else ph } def enteringPhaseNotLaterThanErasure[T](op: => T): T = enteringPhaseNotLaterThan(findPhaseBeforeErasure)(op) ... enteringPhaseNotLaterThanErasure(owner.info.nonPrivateMember(name))

Wouldn't this skip the phase exactly before erasure, too?

Let's leave that determination up to a little test case. If you're right, I'll just say that it only ever intended as pseudocode 😃

Let's hope this is the last time I have to touch this code ... otherwise I'm pulling my hair out. :-)

retronym · 2013-12-12T14:11:32Z

@soc let me know if you need a hand converting this REPL test to a BytecodeTest.

One example were this would lead to subtle bugs otherwise is Symbol_apply, where after erasure an overloaded symbol containing Symbol#apply and UniquenessCache#apply is returned. // findMember considered harmful after erasure; e.g. // // scala> exitingErasure(Symbol_apply).isOverloaded // res27: Boolean = true

Looks like the transformation did never happen because the pattern failed to match. Why did the pattern fail to match? Because the Symbol.apply we see in the tree claims to be a method while Symbol_apply defined in Definitions wants to be a value. This issue was caused because nonPrivateMember starts spitting out overloaded symbols after erasure. This issue has been fixed in the earlier commit, so what happens in this commit is adding tests and fixing documentation.

soc · 2014-01-03T02:09:13Z

@retronym Replaced JavapTest with BytecodeTest. Is this better now?

retronym · 2014-01-09T14:41:24Z

LGTM.

Sorry about the delay in reviewing.

/cc @axel22. I'm curious what sort of code you originally encountered that had bottlenecks from Symbol lookup that warranted this hoisting in the first place. Why not just use string literals in performance sensitive code?

retronym · 2014-01-09T14:49:33Z

test/files/run/t7974/Test.scala

+    val result =
+      classString.split('\n')
+        .dropWhile(elem => elem != "public class Symbols {")
+        .filterNot(elem => elem.startsWith("  @Lscala/reflect/ScalaSignature") || elem.startsWith("  ATTRIBUTE ScalaSig"))


Nitpick: this test could have been a touch more focussed by doing this filtering at the ASM level, rather than with Strings.

Further more, the checkfile contains some irrelevant details, like line numbers (try skipDebugInfo = true next time).

But I'll give you a pass this time as this PR has been spinning for too long.

Fix broken 'Symbol-handling code in CleanUp

In Scala 2.8.2, an optimization was added to create a static cache for Symbol literals (ie, the results of `Symbol.apply("foo"))`. This saves the map lookup on the second pass through code. This actually was broken somewhere during the Scala 2.10 series, after the addition of an overloaded `apply` method to `Symbol`. The cache synthesis code was made aware of the overload and brought back to working condition recently, in scala#3149. However, this has uncovered a latent bug when the Symbol literals are defined with traits. One of the enclosed tests failed with: jvm > t8933b-run.log java.lang.IllegalAccessError: tried to access field MotherClass.symbol$1 from class MixinWithSymbol$class at MixinWithSymbol$class.symbolFromTrait(A.scala:3) at MotherClass.symbolFromTrait(Test.scala:1) This commit simply disables the optimization if we are in a trait. Alternative fixes might be: a) make the static Symbol cache field public / b) "mixin" the static symbol cache. Neither of these seem worth the effort and risk for an already fairly situational optimization. Here's how the optimization looks in a class: % cat sandbox/test.scala; qscalac sandbox/test.scala && echo ":javap C" | qscala; class C { 'a; 'b } Welcome to Scala version 2.11.5-20141106-145558-aa558dce6d (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_20). Type in expressions to have them evaluated. Type :help for more information. scala> :javap C Size 722 bytes MD5 checksum 6bb00189166917254e8d40499ee7c887 Compiled from "test.scala" public class C { public static {}; descriptor: ()V flags: ACC_PUBLIC, ACC_STATIC Code: stack=2, locals=0, args_size=0 0: getstatic #16 // Field scala/Symbol$.MODULE$:Lscala/Symbol$; 3: ldc #18 // String a 5: invokevirtual #22 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol; 8: putstatic #26 // Field symbol$1:Lscala/Symbol; 11: getstatic #16 // Field scala/Symbol$.MODULE$:Lscala/Symbol$; 14: ldc #28 // String b 16: invokevirtual #22 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol; 19: putstatic #31 // Field symbol$2:Lscala/Symbol; 22: return public C(); descriptor: ()V flags: ACC_PUBLIC Code: stack=1, locals=1, args_size=1 0: aload_0 1: invokespecial #34 // Method java/lang/Object."<init>":()V 4: getstatic #26 // Field symbol$1:Lscala/Symbol; 7: pop 8: getstatic #31 // Field symbol$2:Lscala/Symbol; 11: pop 12: return }

In Scala 2.8.2, an optimization was added to create a static cache for Symbol literals (ie, the results of `Symbol.apply("foo"))`. This saves the map lookup on the second pass through code. This actually was broken somewhere during the Scala 2.10 series, after the addition of an overloaded `apply` method to `Symbol`. The cache synthesis code was made aware of the overload and brought back to working condition recently, in scala#3149. However, this has uncovered a latent bug when the Symbol literals are defined with traits. One of the enclosed tests failed with: jvm > t8933b-run.log java.lang.IllegalAccessError: tried to access field MotherClass.symbol$1 from class MixinWithSymbol$class at MixinWithSymbol$class.symbolFromTrait(A.scala:3) at MotherClass.symbolFromTrait(Test.scala:1) This commit simply disables the optimization if we are in a trait. Alternative fixes might be: a) make the static Symbol cache field public / b) "mixin" the static symbol cache. Neither of these seem worth the effort and risk for an already fairly situational optimization. Here's how the optimization looks in a class: % cat sandbox/test.scala; qscalac sandbox/test.scala && echo ":javap C" | qscala; class C { 'a; 'b } Welcome to Scala version 2.11.5-20141106-145558-aa558dce6d (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_20). Type in expressions to have them evaluated. Type :help for more information. scala> :javap C Size 722 bytes MD5 checksum 6bb00189166917254e8d40499ee7c887 Compiled from "test.scala" public class C { public static {}; descriptor: ()V flags: ACC_PUBLIC, ACC_STATIC Code: stack=2, locals=0, args_size=0 0: getstatic #16 // Field scala/Symbol$.MODULE$:Lscala/Symbol$; 3: ldc #18 // String a 5: invokevirtual #22 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol; 8: putstatic #26 // Field symbol$1:Lscala/Symbol; 11: getstatic #16 // Field scala/Symbol$.MODULE$:Lscala/Symbol$; 14: ldc #28 // String b 16: invokevirtual #22 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol; 19: putstatic #31 // Field symbol$2:Lscala/Symbol; 22: return public C(); descriptor: ()V flags: ACC_PUBLIC Code: stack=1, locals=1, args_size=1 0: aload_0 1: invokespecial #34 // Method java/lang/Object."<init>":()V 4: getstatic #26 // Field symbol$1:Lscala/Symbol; 7: pop 8: getstatic #31 // Field symbol$2:Lscala/Symbol; 11: pop 12: return } fixup

A classic mistake of discarding a non-trivial qualifier. We actually should have fixed this before merging scala#3149, as it was raised in review, but I suppose we got too caught up in the difficulty of resolving the right overload of `Symbol_apply` that we forgot.

soc mentioned this pull request Nov 17, 2013

Fixes broken 'Symbol-handling code in CleanUp #3148

Closed

retronym reviewed Nov 24, 2013
View reviewed changes

ghost assigned retronym Dec 13, 2013

retronym reviewed Jan 9, 2014
View reviewed changes

retronym added a commit that referenced this pull request Jan 9, 2014

Merge pull request #3149 from soc/SI-7974

992050a

Fix broken 'Symbol-handling code in CleanUp

retronym merged commit 992050a into scala:master Jan 9, 2014

This was referenced Apr 7, 2017

scala.Symbol's CleanUp code is broken and never used scala/bug#7974

Closed

Unexpected java.lang.IllegalAccessError thrown when accessing symbols in traits with self type scala/bug#8933

Closed

SI-7974 Fix broken 'Symbol-handling code in CleanUp #3149

SI-7974 Fix broken 'Symbol-handling code in CleanUp #3149

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!