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

[Lindenmaier, Goetz]

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.