Quantcast

JDK 8148716: Variable proxy duplicate

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

JDK 8148716: Variable proxy duplicate

B. Blaser
Hi,

Javac fails to compile the following example because it creates twice
the same variable proxy (jdk 8148716):

class A {
    public static void main(String... args) {
        new A().a();
    }

    void a() {
        String x = "abc";

        class B {
            public int b() { return x.length(); }
        };

        Runnable r = () -> {
            class C {
                void c() {
                    // free var x in a()
                    System.out.println(new B().b());
                    // free var x in lambda()
                    System.out.println(x.length());
                }
            }
            new C().c();
        };
        r.run();
    }
}

... once for x in a() and once for x in lambda().

The fix below prevents free variable duplicate regardless of the owner
(Lower.FreeVarCollector.addFreeVar()), ensuring no proxy duplication
(see proxies.getSymbolsByName() in Lower.initField()).

Needs to be well tested...

Any comment is welcome!

Thanks,
Bernard

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
@@ -289,7 +289,7 @@
          */
         private void addFreeVar(VarSymbol v) {
             for (List<VarSymbol> l = fvs; l.nonEmpty(); l = l.tail)
-                if (l.head == v) return;
+                if (l.head.name == v.name) return;
             fvs = fvs.prepend(v);
         }
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 8148716: Variable proxy duplicate

Maurizio Cimadamore
Hi,
I believe that to be a duplicate of

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

for which we have a fix in the works which we'll push to 10 soon.

Srikanth and I decided to go for a more radical rewriting in the area,
making a lot of the logic in the code (now implicit in the use of the
Scope data structure) more explicit and less prone to errors of this sort.

Thanks
Maurizio



On 08/02/17 22:02, B. Blaser wrote:

> Hi,
>
> Javac fails to compile the following example because it creates twice
> the same variable proxy (jdk 8148716):
>
> class A {
>      public static void main(String... args) {
>          new A().a();
>      }
>
>      void a() {
>          String x = "abc";
>
>          class B {
>              public int b() { return x.length(); }
>          };
>
>          Runnable r = () -> {
>              class C {
>                  void c() {
>                      // free var x in a()
>                      System.out.println(new B().b());
>                      // free var x in lambda()
>                      System.out.println(x.length());
>                  }
>              }
>              new C().c();
>          };
>          r.run();
>      }
> }
>
> ... once for x in a() and once for x in lambda().
>
> The fix below prevents free variable duplicate regardless of the owner
> (Lower.FreeVarCollector.addFreeVar()), ensuring no proxy duplication
> (see proxies.getSymbolsByName() in Lower.initField()).
>
> Needs to be well tested...
>
> Any comment is welcome!
>
> Thanks,
> Bernard
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> @@ -289,7 +289,7 @@
>            */
>           private void addFreeVar(VarSymbol v) {
>               for (List<VarSymbol> l = fvs; l.nonEmpty(); l = l.tail)
> -                if (l.head == v) return;
> +                if (l.head.name == v.name) return;
>               fvs = fvs.prepend(v);
>           }

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 8148716: Variable proxy duplicate

B. Blaser
Hi,

2017-02-09 2:00 GMT+01:00 Maurizio Cimadamore <[hidden email]>:

> Hi,
> I believe that to be a duplicate of
>
> https://bugs.openjdk.java.net/browse/JDK-8169345
>
> for which we have a fix in the works which we'll push to 10 soon.
>
> Srikanth and I decided to go for a more radical rewriting in the area,
> making a lot of the logic in the code (now implicit in the use of the Scope
> data structure) more explicit and less prone to errors of this sort.
>
> Thanks
> Maurizio

As this problem seems to be present since jdk6 (issue 8169345 you
pointed me to), it would perhaps be interesting to have a temporary
fix which could be eventually backported, or at least corrected in
jdk9.

To make things more explicit and to have a quite safer patch, I can
suggest to incorporate the name of the variable's owner to the proxy
name, as below. The rewriting you are working on is probably better,
but the following fix could perhaps make things work until jdk10...?

What's your opinion on that?

Thanks,
Bernard

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
@@ -308,7 +308,7 @@
             Symbol sym = _sym;
             if (sym.kind == VAR || sym.kind == MTH) {
                 while (sym != null && sym.owner != owner)
-                    sym = proxies.findFirst(proxyName(sym.name));
+                    sym = proxies.findFirst(proxyName(sym));
                 if (sym != null && sym.owner == owner) {
                     VarSymbol v = (VarSymbol)sym;
                     if (v.getConstValue() == null) {
@@ -1095,7 +1095,7 @@
                 return makeLit(sym.type, cv);
             }
             // Otherwise replace the variable by its proxy.
-            sym = proxies.findFirst(proxyName(sym.name));
+            sym = proxies.findFirst(proxyName(sym));
             Assert.check(sym != null && (sym.flags_field & FINAL) != 0);
             tree = make.at(tree.pos).Ident(sym);
         }
@@ -1394,8 +1394,12 @@

     /** The name of a free variable proxy.
      */
-    Name proxyName(Name name) {
-        return names.fromString("val" + target.syntheticNameChar() + name);
+    Name proxyName(Symbol s) {
+        String sep = target.syntheticNameChar() + "";
+        if (s.owner != null && s.owner.name != null)
+            sep = sep + s.owner.name + sep;
+
+        return names.fromString("val" + sep + s.name);
     }

     /** Proxy definitions for all free variables in given list, in
reverse order.
@@ -1414,7 +1418,7 @@
         for (List<VarSymbol> l = freevars; l.nonEmpty(); l = l.tail) {
             VarSymbol v = l.head;
             VarSymbol proxy = new VarSymbol(
-                flags, proxyName(v.name), v.erasure(types), owner);
+                flags, proxyName(v), v.erasure(types), owner);
             proxies.enter(proxy);
             JCVariableDecl vd = make.at(pos).VarDef(proxy, null);
             vd.vartype = access(vd.vartype);
@@ -2717,7 +2721,7 @@
             if (fvs.nonEmpty()) {
                 List<Type> addedargtypes = List.nil();
                 for (List<VarSymbol> l = fvs; l.nonEmpty(); l = l.tail) {
-                    final Name pName = proxyName(l.head.name);
+                    final Name pName = proxyName(l.head);
                     m.capturedLocals =
                         m.capturedLocals.prepend((VarSymbol)
                                                 (proxies.findFirst(pName)));
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 8148716: Variable proxy duplicate

Maurizio Cimadamore


On 09/02/17 13:02, B. Blaser wrote:

> Hi,
>
> 2017-02-09 2:00 GMT+01:00 Maurizio Cimadamore <[hidden email]>:
>> Hi,
>> I believe that to be a duplicate of
>>
>> https://bugs.openjdk.java.net/browse/JDK-8169345
>>
>> for which we have a fix in the works which we'll push to 10 soon.
>>
>> Srikanth and I decided to go for a more radical rewriting in the area,
>> making a lot of the logic in the code (now implicit in the use of the Scope
>> data structure) more explicit and less prone to errors of this sort.
>>
>> Thanks
>> Maurizio
> As this problem seems to be present since jdk6 (issue 8169345 you
> pointed me to), it would perhaps be interesting to have a temporary
> fix which could be eventually backported, or at least corrected in
> jdk9.
>
> To make things more explicit and to have a quite safer patch, I can
> suggest to incorporate the name of the variable's owner to the proxy
> name, as below. The rewriting you are working on is probably better,
> but the following fix could perhaps make things work until jdk10...?
>
> What's your opinion on that?
Hi
right now it's late to be making risky changes - which is why the fix
was targeted for 10. Once the fix will have been in 10 for some time, we
will consider backporting what we have to 9.

Thanks
Maurizio

>
> Thanks,
> Bernard
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> @@ -308,7 +308,7 @@
>               Symbol sym = _sym;
>               if (sym.kind == VAR || sym.kind == MTH) {
>                   while (sym != null && sym.owner != owner)
> -                    sym = proxies.findFirst(proxyName(sym.name));
> +                    sym = proxies.findFirst(proxyName(sym));
>                   if (sym != null && sym.owner == owner) {
>                       VarSymbol v = (VarSymbol)sym;
>                       if (v.getConstValue() == null) {
> @@ -1095,7 +1095,7 @@
>                   return makeLit(sym.type, cv);
>               }
>               // Otherwise replace the variable by its proxy.
> -            sym = proxies.findFirst(proxyName(sym.name));
> +            sym = proxies.findFirst(proxyName(sym));
>               Assert.check(sym != null && (sym.flags_field & FINAL) != 0);
>               tree = make.at(tree.pos).Ident(sym);
>           }
> @@ -1394,8 +1394,12 @@
>
>       /** The name of a free variable proxy.
>        */
> -    Name proxyName(Name name) {
> -        return names.fromString("val" + target.syntheticNameChar() + name);
> +    Name proxyName(Symbol s) {
> +        String sep = target.syntheticNameChar() + "";
> +        if (s.owner != null && s.owner.name != null)
> +            sep = sep + s.owner.name + sep;
> +
> +        return names.fromString("val" + sep + s.name);
>       }
>
>       /** Proxy definitions for all free variables in given list, in
> reverse order.
> @@ -1414,7 +1418,7 @@
>           for (List<VarSymbol> l = freevars; l.nonEmpty(); l = l.tail) {
>               VarSymbol v = l.head;
>               VarSymbol proxy = new VarSymbol(
> -                flags, proxyName(v.name), v.erasure(types), owner);
> +                flags, proxyName(v), v.erasure(types), owner);
>               proxies.enter(proxy);
>               JCVariableDecl vd = make.at(pos).VarDef(proxy, null);
>               vd.vartype = access(vd.vartype);
> @@ -2717,7 +2721,7 @@
>               if (fvs.nonEmpty()) {
>                   List<Type> addedargtypes = List.nil();
>                   for (List<VarSymbol> l = fvs; l.nonEmpty(); l = l.tail) {
> -                    final Name pName = proxyName(l.head.name);
> +                    final Name pName = proxyName(l.head);
>                       m.capturedLocals =
>                           m.capturedLocals.prepend((VarSymbol)
>                                                   (proxies.findFirst(pName)));

Loading...