[PATCH] 8147527: Non-optimal code generated for postfix unary operators

[bsrbnd]

Hi,

2016-10-25 14:15 GMT+02:00 Jan Lahoda <jan.lahoda at oracle.com>:
> On 25.10.2016 10:41, Maurizio Cimadamore wrote:
>>
>> Hi Jan,
>> great analysis. I think (1) could be a safer path - but I admit I'm not
>> entirely sold on another point; you say:
>>
>> "Further desugaring then replaces the "Issue8147527.super" with "this$0"
>> (see "tree.selected = translate(tree.selected);" in Lower.visitSelect)"
>>
>> Isn't this already wrong? The static type of this$0 is Issue8147527,
>> which is different from the static type of this.super (Parent).
>> Shouldn't javac at least emit a cast there to preserve typing? While I
>> don't dispute the other problems you discovered, which are real, I think
>> the above desugaring doesn't make a lot of sense, and probably only
>> succeeds because the visitor later fixes up the problem (probably by
>> realizing that the owner of the symbol 'i' and the symbol of this$0
>> disagree, therefore adding a cast?)
>
>
> Normally (when the same tree is not reused), this is only a transient state
> inside the visitSelect method - the method keeps a flag
> (qualifiedSuperAccess) that the select expression is a the form "X.super",
> and this will then force generation of an accessor invocation, replacing the
> whole JCFieldAccess with something like "Issue8147527.access$101(this$0)".
> But in case of reused trees, this will only correct/replace the first use of
> the tree, and the subsequent one will remain broken.
>
If "tree.selected=..." appears to be a wrong transient state, then
avoiding it (option 2 that Jan suggested) depending on the
"qualifiedSuperAccess" flag and forcing the accessor generation could
be the best approach.

The fix, here below, shows the idea...

I know it is not perfect, but as it corrects this specific case (and
all test/tools/javac/boxing passed), I would like to know your opinion
about it.
Is this a good direction to search?

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
@@ -145,6 +145,7 @@
     /** A hash table mapping local classes to their definitions.
      */
     Map<ClassSymbol, JCClassDecl> classdefs;
+    Map<ClassSymbol, JCClassDecl> allclassdefs = new HashMap<>();

     /** A hash table mapping local classes to a list of pruned trees.
      */
@@ -210,6 +211,8 @@
             classMap.scan(outermostClassDef);
             def = classdefs.get(c);
         }
+        if (def == null)
+            def = allclassdefs.get(c);
         return def;
     }

@@ -1100,6 +1103,7 @@
             tree = make.at(tree.pos).Ident(sym);
         }
         JCExpression base = (tree.hasTag(SELECT)) ? ((JCFieldAccess)
tree).selected : null;
+        base = base != null ? translate(base) : null;
         switch (sym.kind) {
         case TYP:
             if (sym.owner.kind != PCK) {
@@ -1300,7 +1304,9 @@
                 //odd access codes represent qualified super accesses - need to
                 //emit reference to the direct superclass, even if the refered
                 //member is from an indirect superclass (JLS 13.1)
-
site.setType(types.erasure(types.supertype(vsym.owner.enclClass().type)));
+                Type supertype = types.supertype(vsym.owner.enclClass().type);
+                if (!supertype.equals(syms.objectType))
+                    site.setType(types.erasure(supertype));
             }
             ref = make.Select(site, sym);
             args = make.Idents(md.params.tail);
@@ -2413,6 +2419,7 @@
             attrEnv = prevEnv;

         classdefs.put(currentClass, tree);
+        allclassdefs.put(currentClass, tree);

         proxies = proxies.dup(currentClass);
         List<VarSymbol> prevOuterThisStack = outerThisStack;
@@ -3837,7 +3844,7 @@
             tree.selected.hasTag(SELECT) &&
             TreeInfo.name(tree.selected) == names._super &&
             !types.isDirectSuperInterface(((JCFieldAccess)tree.selected).selected.type.tsym,
currentClass);
-        tree.selected = translate(tree.selected);
+        if (!qualifiedSuperAccess) tree.selected = translate(tree.selected);
         if (tree.name == names._class) {
             result = classOf(tree.selected);
         }