RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.

[coleen.phillimore at oracle.com]

This last version looks good to me.
Coleen

On 9/17/19 5:23 AM, Lindenmaier, Goetz wrote:
> Hi Coleen,
>
> thanks for having another look at this change.
>
> new webrev:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/17-incremental/
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/17/
>
>> Can you change the comment blocks to // comments?
> Oops, sure! Fixed.
>
>> 1377     // Print string describing which action (bytecode) could not be
>> 1378     // performed because of the null reference.
>> 1379     emb.print_NPE_failed_action(ss, bci);
>> 1380     // Print a description of what is null.
>> 1381     if (!emb.print_NPE_cause(ss, bci, slot)) {
>> 1382       // Nothing was printed. End the sentence without the 'because'
>> 1383       // subordinate sentence.
>> 1384       ss->print(".");
>> 1385     }
>>
>>
>> I think you should have a ResourceMark here.
> Before, a resource mark in bytecodeUtils would have broken
> the stringStream passed in.
> But as Thomas pointed out, that restriction has been removed.
> Thus, I could move the ResourceMark into this method.
>
>> 1099   assert(bci >= 0, "BCI too low");
>> 1100   assert(bci < get_size(), "BCI too large");
>   
>> I think sanity checking the bci should be in the beginning of the
>> ExceptionMessageBuilder constructor, or even in the main function before
>> the constructor call.  This seems too late; after it's done all the work.
> print_NPE_cause0 calls itself recursively with different bcis.  The bcis have
> been computed by the analysis, so they should be sane, but this is to protect
> against errors there.
> The same assertion in the constructor makes sense, too. There, the bci
> passed to the algorithm will be checked for sanity. I added the assert there,
> too.
>
>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
>> NPE/16/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/Null
>> PointerExceptionTest.java.html
>>
>> I think we're supposed to put @bug 8218628 in the test (even if it's an
>> RFE).
> Added in all three tests.
> Also improved some of the comments in the tests slightly.
>
>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
>> NPE/16/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPE
>> InHiddenTopFrameTest.java.html
>>
>>     31  * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:-
>> ShowHiddenFrames -XX:-ShowCodeDetailsInExceptionMessages
>> NPEInHiddenTopFrameTest hidden
>>
>> Shouldn't this be -XX:+ShowCodeDetailsInExceptionMessages to show that
>> the hidden frame isn't printed with the new code?
> You are right!  The test passes, too with '-', but that is not what
> is intended to be tested! Fixed.  Nice catch!
>
>> I've done a few passes of reviewing this code.  These were the only new
>> things that I noticed.  Thank you for making the names more descriptive
>> in BytecodeUtils.   This looks good to me!   Hope that people will love
>> this feature!
> Thanks a lot!
> I think I should get a review on core-libs-dev for the changes to
> NullPointerException.java.
>
>> Thank you Goetz for all your hard work!
>>
>> Coleen
>>
>>
>> On 9/10/19 10:44 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> the subject should mention 8218628. (Not 8218626).
>>> Sorry for this!
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>> From: Lindenmaier, Goetz
>>> Sent: Dienstag, 10. September 2019 11:48
>>> To: 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>; Java
>> Core Libs <core-libs-dev at openjdk.java.net>
>>> Subject: RFR (L, final): 8218626: Add detailed message to
>> NullPointerException describing what is null.
>>> Hi,
>>>
>>> this is the implementation of JEP 358: Helpful NullPointerExceptions.
>>>
>>> The JEP is in status "Candidate". Coleen (many, many thanks!) ran
>>> it through the Oracle-internal processes.  Now I please need final reviews
>>> for this change so that I can propose it to target jdk 14.
>>>
>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
>>> Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628
>>> webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/
>>>
>>> The change ran through a lot of testing, all jtreg and jck tests to name
>>> only some. The webrev points to patch
>>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
>> NPE/16/enable_NPE_message.patch
>>> that enables the change by default,  which was useful for testing to
>>> assure the code is used in the tests.
>>> I just pushed the change to jdk/submit once more.
>>>
>>> Please review.
>>>
>>> Thanks!
>>>     Goetz.