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

[bsrbnd]

I've noticed another esoterism in Inner.testParent().

The proposed fix places the accessor for field "i" in Parent:

  private java.lang.Integer testParent();
    Code:
       0: aload_0
       1: getfield      #3                  // Field this$0:LIssue8147527;
       4: invokestatic  #8                  // Method
Parent.access$201:(LParent;)Ljava/lang/Integer;
       7: astore_1

Prior to that, it was placed in Issue8147527:

  private java.lang.Integer testParent();
    Code:
       0: aload_0
       1: getfield      #3                  // Field this$0:LIssue8147527;
       4: invokestatic  #8                  // Method
Issue8147527.access$201:(LIssue8147527;)Ljava/lang/Integer;
       7: astore_1                          // !!! REFERENCE TO VALUE
OF Field Parent.i:Ljava/lang/Integer; !!!

And before issue 8143388 changeset, it was accessed directly:

  private java.lang.Integer testParent();
    Code:
       0: aload_0
       1: getfield      #3                  // Field this$0:LIssue8147527;
       4: astore_1
       5: aload_1
       6: getfield      #8                  // Field
Parent.i:Ljava/lang/Integer;
       9: astore_2

What's the expected accessing way?

Bernard


2016-10-26 12:22 GMT+02:00 bsrbnd <bsrbnd at gmail.com>:
> 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);
>          }