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

[Mandy Chung]

Hi Goetz,

On 3/15/19 3:55 AM, Lindenmaier, Goetz wrote:
> I followed your advice and created a JEP:
> https://bugs.openjdk.java.net/browse/JDK-8220715

This is a good start.   I include my comments as a reader who does not
read TrackingStackCreator and other C++ code.

In the "Basic algorithm to compute the message" section:

"This dataflow analysis walks the bytecodes forward simulating the 
execution stack."

Please elaborate it.  Given a stack trace, it starts with the most recently
executed method and bytecode index, I assume it simulates the method from
the beginning of the method until it hits the instruction with the 
matching BCI.
Please describe it and also the cases when it requires to look at its 
caller frame.

"Computing either part of these messages can fail due to insufficient 
information."

What are the cases that it fails to obtain the information? It'd be useful
to list some examples if not all.

The "Different bytecode variants of a method" section

This section raises the case when a method representing an execution stack
frame throwing NPE becomes obsolete, it does not have the information
to compute the message lazily.  So this falls into the "insufficient
information" category and no improved NPE message can be generated.

The "computation overhead" section

I agree with the requirement to have minimal performance overhead to
the NPE construction cost.

"NullPointerExceptions are thrown frequently."
"Fortunately, most Exceptions are discarded without looking at the message."

This is interesting.  Do you have any statistic on the number of NPE thrown
and swallowed on certain application/environment that you have gathered?

"If the message is printed based on the backtrace, it must be omitted
  if the top frame was dropped from the backtrace."

If NPE includes the hidden frames, `Throwable::getStackTrace` and
`Throwable::toString` and the VM printing of NPE stack trace needs to
filter the hidden frames.  I think it's appropriate for this JEP to
cover rather than in a separate JBS issue.

The "Message persistance" section

I read this section like an implementation details.  I think this section
intends to call out that the message of NPE if constructed by user code
i.e. via public constructor, should not be altered including no-arg
constructor or pass a null message to 1-arg constructor.  The enhanced
NPE message is proposed only when NPE is thrown due to null dereference
when executing bytecode.

I don't understand what you tried to say about "npe.getMessage() == 
npe.getMessage()".
Throwable::getMessage does not guarantee to return same String object for
each invocation.

When deserializing the class bytes may be of a different version, so 
StackTraceElements
are serialized with its string form.  Serializing NPE with a computed 
message
seems to be the only viable solution.

The "Message content" section

I think this section can break down into clear scenarios
1. debug information is present
2. debug information is absent
3. the matching version of bytecode is not available

I agree that the message should be easy for Java developers including the
beginners to understand the error.

You bring up a good question about the exception message format e.g. 
whether with
quotation (none vs single quote vs double quote).  It's good to have a 
guideline
on that while this is out of scope of this JEP.  IAE was updated to include
module name, loader name and improve the error message for the developers
to understand the exception which is a good improvement.

"Alternatives" section

"The current proposal is to implement this in the Java runtime in C++ 
accessing
  directly the available datastructures in the metaspace."

"Also, ASM and StackWalker do not support the full functionality needed.
StackWalker must be called in the NullPointerException constructor."

This is not true as you wrote in the subsequent sentence.  If we choose
the implementation to Java, StackWalker will need to be extended to
read Throwable's backtrace.  Such specialized form can provide the
ability to fetch the bytecode in Method* if appropriate.  This is
the implementation details.  As discussed in the email thread, another
implementation choice could be a hybrid of native in the VM and Java
to accomplish this task (for example fetching the bytecode of the
current version could be done in the VM if we agree supporting
the redefinition case).

For logic that is not strictly needed to be done in the VM, doing it
in Java and library is a good option to consider (putting aside the
amount of new code you have to write).   Looks like you can't evaluate
the alternatives since existing library code requires work in order to
prototype this in Java.  So I'd say more investigation needs to be done
to decide on alternative implementation approaches.

"Testing" section

"The basic implementation is in use in SAP's internal Java virtual 
machine since 2006."

This is good information.  This assumes if the same implementation goes 
into JDK.
So this is subject to the discussion and code review.   I think it's 
adequate to say unit tests
will need to be developed.  I think this feature should not impact much 
of the existing VM code
and so running existing jtreg tests, jck tests and other existing test 
suites should provide
adequate coverage.

I found [1] is quite useful to understand the scenarios this JEP 
considers.  I think it
would be useful to include them in the JEP perhaps as examples when 
describing
the data flow analysis.  [1] can be grouped into several categories and 
the JEP can
just talk about the worthnoting groups and does not need to list all 
messages such as
dereference a null receive on getfield, null array element vs null array

This JEP includes a few links to the C++ functions in the current 
webrev.  I appreciate
that and I assume they will be taken out at some point.

Mandy