[PATCH] 8147527: Non-optimal code generated for postfix unary operators
bsrbnd
bsrbnd at gmail.com
Thu Oct 27 17:33:21 UTC 2016
I finally put the accessor in class "Issue8147527" as explained in
Lower.accessSymbol() first comment.
The fix looks quite good now (I hope)...
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
@@ -926,7 +926,7 @@
ClassSymbol accOwner = refSuper && protAccess
// For access via qualified super (T.super.x), place the
// access symbol on T.
- ? (ClassSymbol)((JCFieldAccess) tree).selected.type.tsym
+ ? (ClassSymbol)((JCFieldAccess)((JCFieldAccess)
tree).selected).selected.type.tsym
// Otherwise pretend that the owner of an accessed
// protected symbol is the enclosing class of the current
// class which is a subclass of the symbol's owner.
@@ -1100,6 +1100,7 @@
tree = make.at(tree.pos).Ident(sym);
}
JCExpression base = (tree.hasTag(SELECT)) ? ((JCFieldAccess)
tree).selected : null;
+ base = refSuper ? translate(base) : base;
switch (sym.kind) {
case TYP:
if (sym.owner.kind != PCK) {
@@ -3837,7 +3838,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);
}
2016-10-27 15:33 GMT+02:00 bsrbnd <bsrbnd at gmail.com>:
> 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);
>> }
More information about the compiler-dev
mailing list