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

[Vicente Romero]

On 02/24/2018 03:39 PM, B. Blaser wrote:
> 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.

sure, I understand, I will take care of it. And yes your work is very 
helpful,

Thanks,
Vicente

>
> 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);
>>