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

[Lindenmaier, Goetz]

Hi Christoph, 

thanks for looking at the code!

I incorporated your proposed changes, see these webrevs:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18-incremental/

Best regards,
  Goetz.

> -----Original Message-----
> From: Langer, Christoph <christoph.langer at sap.com>
> Sent: Donnerstag, 19. September 2019 23:35
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> coleen.phillimore at oracle.com; hotspot-runtime-dev at openjdk.java.net
> Subject: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
> 
> 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.