RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Coleen,

> Backtrace elements don't keep the Method* alive.
You are right, it's not kept alive. If the method is gone, 
get_method_and_bci() will return false here:
    http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-otherMessages/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
  +  Method* act_method = holder->method_with_idnum(method_id);
  +  if (act_method == NULL) {
  +    return false;
  +  }
and no message will be printed. So we are safe from printing
a misleading message.

Best regards,
  Goetz.


> -----Original Message-----
> From: hotspot-runtime-dev <hotspot-runtime-dev-
> bounces at openjdk.java.net> On Behalf Of coleen.phillimore at oracle.com
> Sent: Dienstag, 19. März 2019 16:08
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> 
> 
> On 3/18/19 6:01 AM, Lindenmaier, Goetz wrote:
> > Hi Mandy,
> >
> >> ... This brings up one
> >> issue that when constructing the enhanced NPE message, a method
> >> might become obsolete due to redefintion, it should handle
> >> gracefully.
> > The C++ solution has access to the proper version of redefined codes,
> > so this is no problem.  It is attached to the backtrace.
> > And yes, in the Java variant we could just skip the message if we don't
> > have the proper variant of the bytecode at hand. But we need
> > a way to find out that this is the case.  It's unclear to me how this
> > should work ... it also depends on the way we retrieve the bytecodes
> > to analyze.
> 
> The Java variant of retrieving the bytecode would depend on
> java.lang.Class.redefinedCount but this is racy.  You can test the
> redefinition count, and in the next bytecode redefine the class. So you
> have to be careful to retest the redefinedCount after getting the bytecode.
> Since this is a corner case, the C++ code doesn't try to "find" the old
> bytecode because it may be in a Method* that has been deallocated.
> Backtrace elements don't keep the Method* alive.
> 
> Thanks,
> Coleen
> 
> 
> >
> > Best regards,
> >    Goetz.
> >
> >
> >
> >> Mandy
> >>
> >> On 3/15/19 10:34 AM, Maurizio Cimadamore wrote:
> >>> You touch an important point here - who is this feature for? Is it for
> >>> the casual developer in need of some hand holding? Or is it for
> >>> diagnosing complex instrumented stacks? I think the two use cases are
> >>> not necessarily similar and might be served by different sets of
> >>> enhancements. In the latter case it might be ok, for instance, to say
> >>> "and, btw, the NPE came from local[1]". But for users in the former
> >>> bucket (and most users, really), this level of detail will simply be
> >>> unacceptable (since now they have to understand the JVMS to parse the
> >>> message :-)).
> >>>
> >>> I suggest we separate the use cases, at least for the purpose of the
> >>> design discussion.
> >>>
> >>> Maurizio
> >>>
> >>> On 15/03/2019 13:11, Lindenmaier, Goetz wrote:
> >>>> Hi Maurizio,
> >>>>
> >>>> thanks for sharing your patch!  This is about what I thought about
> >>>> so far, just that it's working already :)
> >>>>
> >>>> It prints the information of the failing bytecode.  Adding the dataflow
> >>>> analysis, the other information could be printed as well.
> >>>>
> >>>> The major problem I see is here:
> >>>> +            File f = new File(location.getFile(),
> >>>> +                    clazz.getCanonicalName().replaceAll("\\.", "/") +
> >>>> ".class");
> >>>> +            ClassReader cr = new ClassReader(new FileInputStream(f));
> >>>> +            ClassNode classNode = new ClassNode();
> >>>> +            cr.accept(classNode, 0);
> >>>>
> >>>> As I understand, this reads the Bytecode from the classfile.
> >>>> The bytecode in the classfile must not match that executed
> >>>> by the VM, i.e, the pc you get from the stackWalker will not
> >>>> reference the bytecode that caused the NPE.
> >>>>
> >>>> Other issues have been discussed in the mail thread before.
> >>>> Good that you point this out, I'll add it to the JEP.
> >>>>
> >>>> Best regards,
> >>>>     Goetz
> >>>>
> >>>> ... Sorry, I missed the "reply all" on my first reply.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Maurizio Cimadamore <maurizio.cimadamore at oracle.com>
> >>>>> Sent: Freitag, 15. März 2019 12:33
> >>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Mandy Chung
> >>>>> <mandy.chung at oracle.com>; Roger Riggs <Roger.Riggs at oracle.com>
> >>>>> Cc: Java Core Libs <core-libs-dev at openjdk.java.net>; hotspot-runtime-
> >>>>> dev at openjdk.java.net
> >>>>> Subject: Re: RFR(L): 8218628: Add detailed message to
> >>>>> NullPointerException
> >>>>> describing what is null.
> >>>>>
> >>>>> Hi Goetz,
> >>>>> please find the attached ASM-based patch. It is just a PoC, as such it
> >>>>> does not provide as fine-grained messages as the one discussed in the
> >>>>> RFE/JEP, but can be enhanced to cover custom debugging attribute, I
> >>>>> believe.
> >>>>>
> >>>>> When running this:
> >>>>>
> >>>>> Object o = null;
> >>>>> o.toString();
> >>>>>
> >>>>> you get:
> >>>>>
> >>>>> Exception in thread "main" java.lang.NullPointerException: attempt to
> >>>>> dereference 'null' when calling method 'toString'
> >>>>>        at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)
> >>>>>
> >>>>> While when running this:
> >>>>>
> >>>>> Foo foo = null;
> >>>>> int y = foo.x;
> >>>>>
> >>>>> You get this:
> >>>>>
> >>>>> Exception in thread "main" java.lang.NullPointerException: attempt to
> >>>>> dereference 'null' when accessing field 'x'
> >>>>>        at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)
> >>>>>
> >>>>> One problem I had is that ASM provides no way to get the instruction
> >>>>> given a program counter - which means we have to scan all the
> bytecodes
> >>>>> and update the sizes as we go along, and, ASM unfortunately doesn’t
> >>>>> expose opcode sizes either. A more robust solution would be to have a
> >>>>> big switch which returned the opcode size of any given opcode. Also,
> >>>>> accessing to StackWalker API on exception creation might not be
> >>>>> desirable in terms of performances, so this might be one of these area
> >>>>> where some VM help could be beneficial. Another problem is that we
> >>>>> cannot distinguish between user-generated exceptions (e.g. `throw new
> >>>>> NullPointerException`) and genuine NPE issued by the VM.
> >>>>>
> >>>>> But I guess the upshot is that it works to leave all the gory detail of
> >>>>> bytecode grovelling to a bytecode API - if the logic is applied lazily,
> >>>>> then the impact on performances should be minimal, and the solution
> >> more
> >>>>> maintainable longer term.
> >>>>>
> >>>>> Cheers
> >>>>> Maurizio
> >>>>>
> >>>>> On 15/03/2019 07:59, Lindenmaier, Goetz wrote:
> >>>>>> Yes, it would be nice if you shared that.