RFR: 8238735 NPE compiling lambda expression within conditional expression

[Maurizio Cimadamore]

Pushed

Maurizio

On 17/06/2020 15:15, Adam Sotona wrote:
> Yes, into jdk/jdk15
>
> Thanks,
> Adam
>
>> On 17 Jun 2020, at 15:51, Maurizio Cimadamore 
>> <maurizio.cimadamore at oracle.com 
>> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>>
>>
>> On 17/06/2020 12:23, Adam Sotona wrote:
>>> Hi Maurizio,
>>> thanks for the review.
>>>
>>> May I ask you to push the patch?
>>> Here is the changeset version of the patch in webrev: 
>>> http://cr.openjdk.java.net/~asotona/8238735/webrev.01b/
>>
>> I can push - I assume this needs to go straight into jdk/jdk15 and 
>> not to jdk/jdk, right?
>>
>> Maurizio
>>
>>>
>>> Thank you,
>>> Adam
>>>
>>>> On 17 Jun 2020, at 11:54, Maurizio Cimadamore 
>>>> <maurizio.cimadamore at oracle.com 
>>>> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>>>>
>>>> The patch looks less invasive/more pragmatic.
>>>>
>>>> Looks good.
>>>>
>>>> Maurizio
>>>>
>>>> On 17/06/2020 08:37, Adam Sotona wrote:
>>>>> Hi,
>>>>> I've spent some time deep debugging of the javac NPE caused by 
>>>>> lambda recovery process and here is an alternate patch (in 
>>>>> contrast to the previous proposed patch skipping the recovery for 
>>>>> the case).
>>>>> webrev: http://cr.openjdk.java.net/~asotona/8238735/webrev.01/
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8238735
>>>>>
>>>>> This patch runs the recovery round in case of invalid 
>>>>> FunctionDescriptor, however to avoid return of result containing 
>>>>> Type.recoveryType (which has no tsym and so causing the NPE and 
>>>>> compiler crash) the final returned type is saved error type from 
>>>>> the first round of visitLambda. The patch also contains new test 
>>>>> for this NPE case.
>>>>>
>>>>> The patch passes all Tier1, Tier2 and Tier 3 tests.
>>>>>
>>>>> Thanks for the review,
>>>>> Adam
>>>>>
>>>>>
>>>>>> On 20 May 2020, at 17:25, Adam Sotona <adam.sotona at oracle.com 
>>>>>> <mailto:adam.sotona at oracle.com>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>> in this situation the second "recovery" round completely messes 
>>>>>> the results.
>>>>>> In the first run the result is correctly filled: result = 
>>>>>> that.type = types.createErrorType(pt()); and suppose to be returned,
>>>>>> however then another recovery is triggered and it  re-enters the 
>>>>>> whole visitLambda with totally messed initial values.
>>>>>> As a result the second evaluation returns Type.recoveryType 
>>>>>> constant, which I expect should be never returned and with no 
>>>>>> indication that the evaluation failed.
>>>>>> Type.recoveryType has missing .tsym field and that is causing NPE.
>>>>>> My original idea was to simply avoid NPE by appropriate check, 
>>>>>> however I do not see any such similar code across javac.
>>>>>> I see many direct calls of Type.tsymin the code however no NPE 
>>>>>> protection anywhere. How it is guaranteed that instances 
>>>>>> of JCNoType (with null tsym) will never reach the code?
>>>>>>
>>>>>> Then I tried to fix it by correcting the result of the recovery 
>>>>>> round, however that seems to be very complex as the initial 
>>>>>> values of the recovery may vary a lot and detect the "messed" 
>>>>>> situation and find what suppose to be returned in that case is 
>>>>>> very complex.
>>>>>>
>>>>>> Then I realized that this is the situation where recovery should 
>>>>>> not happen as lambda without its FunctionDescriptor can hardly 
>>>>>> continue in compilation as it does not know anything about the 
>>>>>> expected content.
>>>>>>
>>>>>> The two error messages I had to remove from the test after the 
>>>>>> fix didn't make any sense to me. From all aspect there are two 
>>>>>> errors in the two lambdas, however second round of recovery 
>>>>>> listed two more nonsense errors: 
>>>>>> "compiler.err.cant.resolve.location: kindname.class, 
>>>>>> NonExistentClass, , , (compiler.misc.location: kindname.class, 
>>>>>> TargetType43, null)"
>>>>>>
>>>>>> BTW: Impact of the NPE is not just when used within conditional 
>>>>>> expression (as described in the 8238735), however it is silently 
>>>>>> killing javac with NPE anywhere you try to cast lambda expression 
>>>>>> that fails to lookup its FunctionDescriptor. For example just try 
>>>>>> to compile: class A {boolean b = (boolean)(() -> false);}
>>>>>>
>>>>>> Thanks,
>>>>>> Adam
>>>>>>
>>>>>>> On 20 May 2020, at 10:19, Adam Sotona <adam.sotona at oracle.com 
>>>>>>> <mailto:adam.sotona at oracle.com>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>> I would like to add results from Corpus compilation regression 
>>>>>>> testing: the patch caused no regression in compilation of all 
>>>>>>> Corpus sources :)
>>>>>>>
>>>>>>> Thanks for review,
>>>>>>> Adam
>>>>>>>
>>>>>>>> On 18 May 2020, at 17:47, Adam Sotona <adam.sotona at oracle.com 
>>>>>>>> <mailto:adam.sotona at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>> I would like to ask for review of the patch fixing NPE while 
>>>>>>>> compiling lambda expression within conditional expression.
>>>>>>>> The fix removes obsolete recovery cycle in 
>>>>>>>> com.sun.tools.javac.comp.Attr.visitLambda method when 
>>>>>>>> Types.FunctionDescriptorLookupError happens and it adds new test.
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8238735
>>>>>>>> webrev: http://cr.openjdk.java.net/~asotona/8238735/webrev.00/
>>>>>>>>
>>>>>>>> Mach 5 build with the patch passes all Tier1, Tier2 and Tier3 
>>>>>>>> tests.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Adam
>>>>>>>
>>>>>>
>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20200617/56731f41/attachment.htm>