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

[Langer, Christoph]

Hi Götz,

I've reviewed merely the java parts of your change. Those look quite good. I've attached a little patch that would fixup some Java warnings in the tests. You could try to run the tests with these amendments and when they still work qfold them into your patch. Other than that, the java file changes look good (it's probably the least part of the whole changeset).

In the C/C++ part, there's only one minor thing that caught my eye: you could remove the blank line 197 in jvm.h �� But in that area I didn't spend too much of effort to think into every detail.

Best regards
Christoph

> -----Original Message-----
> From: hotspot-runtime-dev <hotspot-runtime-dev-
> bounces at openjdk.java.net> On Behalf Of Lindenmaier, Goetz
> Sent: Dienstag, 17. September 2019 11:23
> To: coleen.phillimore at oracle.com; hotspot-runtime-dev at openjdk.java.net
> Subject: [CAUTION] RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
> 
> 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/N
> ull
> > 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/N
> PE
> > 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.