JDK-8189335: NPE in Lower due to class name clash

[B. Blaser]

Hi Vicente,

Thanks for your answer.

On 23 February 2018 at 22:24, Vicente Romero <vicente.romero at oracle.com> wrote:
> Hi Bernard,
>
> Usually when we touch this area of the compiler we do the following test:
> build the whole JDK, another big corpus could make it too, with and without
> the fix applied and then we compare the class files obtained, if no new tag
> classes are generated for cases where they shouldn't and if there are no
> unjustified changes to the rest of the classes, then we are more confident
> about the fix.

You're right, this would be a very good test but I probably won't have
time to do that soon.
I also know that all jdk tests have to pass successfully and I only
ran javac tests, sorry...
However, I hope my work will still be helpful.

Bernard

> Thanks,
> Vicente
>
>
> On 02/22/2018 03:09 PM, B. Blaser wrote:
>>
>> On 21 February 2018 at 16:58, B. Blaser <bsrbnd at gmail.com> wrote:
>>>
>>> On 19 February 2018 at 15:53, B. Blaser <bsrbnd at gmail.com> wrote:
>>>>
>>>> On 19 February 2018 at 00:42, B. Blaser <bsrbnd at gmail.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> As explained in JDK-8189335, the following example crashes javac with
>>>>> a NPE in Lower:
>>>>>
>>>>> class R {
>>>>>      private class R$1 {}
>>>>>      void f() {
>>>>>          new R$1();
>>>>>      }
>>>>> }
>>>>> class R$1 {}
>>>
>>> Some precisions related to my laconic explanation about the problem and
>>> the fix.
>>>
>>> All starts in 'Lower.accessConstructorTag()' when searching for the
>>> anonymous class 'R$1':
>>>
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l1238
>>>
>>> The top level class 'R$1' (not anonymous) is found and wrongly added
>>> to 'accessConstrTags'.
>>>
>>> Then, when checking the access constructor tags in
>>> 'Lower.checkAccessConstructorTags()', the 'outermostClass()' of the
>>> top level class 'R$1' is requested:
>>>
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l778
>>>
>>> 'R$1' being a top level class, its outermost class doesn't exist,
>>> causing the NPE in 'Lower.makeEmptyClass()'.
>>
>> Errata: 'R$1' being a top level class, its outermost class definition
>> ('R$1') doesn't exist within 'R', causing the NPE in
>> 'Lower.makeEmptyClass()':
>>
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l658
>>
>>> The fix I suggested two days ago (here under) is then quite
>>> straightforward. If a class 'R$1' exists but isn't anonymous, it has
>>> to be skipped and a fresh anonymous class 'R$2' has to be created as
>>> access constructor tag:
>>>
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/02404e27d356/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l1239
>>>
>>> Nothing seems to be broken and the provided example compiles
>>> successfully.
>>
>> Here is a small variant of the previous patch that makes sure to reuse
>> the same tag when multiple private constructors are present and 'R$1'
>> isn't anonymous, for example:
>>
>> class R {
>>      private class A {}
>>      private class B {}
>>      void f() {
>>          new A();
>>          new B();
>>      }
>> }
>> class R$1{}
>>
>> Any comment or review is welcome!
>>
>> Cheers,
>> 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
>> @@ -1232,15 +1232,19 @@
>>       ClassSymbol accessConstructorTag() {
>>           ClassSymbol topClass = currentClass.outermostClass();
>>           ModuleSymbol topModle = topClass.packge().modle;
>> -        Name flatname = names.fromString("" + topClass.getQualifiedName()
>> +
>> -                                         target.syntheticNameChar() +
>> -                                         "1");
>> -        ClassSymbol ctag = chk.getCompiled(topModle, flatname);
>> -        if (ctag == null)
>> -            ctag = makeEmptyClass(STATIC | SYNTHETIC, topClass).sym;
>> -        // keep a record of all tags, to verify that all are
>> generated as required
>> -        accessConstrTags = accessConstrTags.prepend(ctag);
>> -        return ctag;
>> +        for (int i = 1; ; i++) {
>> +            Name flatname = names.fromString("" +
>> topClass.getQualifiedName() +
>> +                                            target.syntheticNameChar() +
>> +                                            i);
>> +            ClassSymbol ctag = chk.getCompiled(topModle, flatname);
>> +            if (ctag == null)
>> +                ctag = makeEmptyClass(STATIC | SYNTHETIC, topClass).sym;
>> +            else if (!ctag.isAnonymous())
>> +                continue;
>> +            // keep a record of all tags, to verify that all are
>> generated as required
>> +            accessConstrTags = accessConstrTags.prepend(ctag);
>> +            return ctag;
>> +        }
>>       }
>>
>>       /** Add all required access methods for a private symbol to
>> enclosing class.
>>
>>> What do you think (I hope this is clearer)?
>>>
>>> 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
>>>> @@ -1236,7 +1236,7 @@
>>>>                                            target.syntheticNameChar() +
>>>>                                            "1");
>>>>           ClassSymbol ctag = chk.getCompiled(topModle, flatname);
>>>> -        if (ctag == null)
>>>> +        if (ctag == null || !ctag.isAnonymous())
>>>>               ctag = makeEmptyClass(STATIC | SYNTHETIC, topClass).sym;
>>>>           // keep a record of all tags, to verify that all are
>>>> generated as required
>>>>           accessConstrTags = accessConstrTags.prepend(ctag);
>
>