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

Tue Feb 12 13:14:40 UTC 2019

Hi Roger, 

thanks for looking at my change!

> A few higher level issues should be considered, though the details of
> the webrev captured my immediate attention.
> 
> Is this the right feature
Yes!!

>  and is this the right level of implementation (C++/native)?
See below.

>  From a security perspective, adding field names to exceptions exposes
> information to callers that they could not otherwise get and that breaks
> encapsulation.
> That needs to be evaluated.
I can not comment on that. How can I trigger an evaluation of this?
Who needs to evaluate this?

> I think there are ways to make this easier to implement and be easier to
> maintain and perform better.
>
> NullPointerException:
> 28: unused import of Field
Removed

> 64: The lazyComputeMessage boolean should be inverted so it does not
> require
>     initialization, false is the default, anything else adds overhead.
>     And it may not be needed.
Peter Levart proposed to initialize the message with a sentinel instead.
I'll implement this as a proposal.

> 91: Why do you think synchonization is necessary?  It is a performance hit.
>     It is highly unlikely to be called from multiple threads and the
> value will
>     be the same whether it is computed once or multiple times by
> different threads.
I guess we can go without. 
It would be possible to construct a case where two threads
do getMessage() on the same NPE Object but the string is not 
the same.

> 99: extendedMessage will never be null (so says the javadoc of
> getExtendedNPEMessage)
>    Bug: If it does return null, then null is returned from getMessage
>    regardless of the value of super.getMessage().
Fixed.  

> 121: Simplify getExtendedNPEMessage by making it only responsible for
> the detail
>    and not concatenation with an existing message.  Put that in
> getMessage().
Fixed. You are right, I only call this when the message is NULL.

>    Its not strictly necessary to change the updated message with
> setMessage().
>    Avoiding that will avoid complexity and a performance hit.
>    The message will be computed each time it is needed, and that happens
> infrequently.
>    (Even in the serialization case, it will be re-computed from the
> attached stack frames).
No, you can not recompute it from the stacktrace because that
does not contain the BCI. Also, after deserialization, the bytecode
might look different or not available at all. It depends on the actual 
bytecode that has been running when the exception was thrown.

>    And it avoids adding setMessage() to Throwable, that's significant
> change since
>    the current implementation only allows the message to be initialized
> not updated.
>    Immutable is an important characteristic to maintain.
Yes, I don't like I have to set this. But it follows the same pattern
as with the stack trace which is only computed on demand. Thus, 
Throwable is not immutable anyways.  Before, I implemented this by a 
JNI call hiding this in the Java code.  
I proposed implementing setting the field by reflection, Christoph
proposed a shared secret.  David favored the package private setter.
Please advice what is best.

>  That makes the writeReplace() unnecessary too.
No. I need this, see above.

> Additional command line arguments are an unnecessary complexity,
> documentation, and maintenance overhead.  If the feature is as valuable as
> you suggest, it should be enabled all the time.
Removed.  

> I'm assuming there are no tests broken by the modification of the message.
> It is likely that somewhere an application or test looks at the message
> itself.
> Has that been verified?
Our nightly testing runs the headless jdk and hostspot jtreg tests, jck tests and some
other applications. They are all unaffected by this change after adapting the 
message in jtreg/vmTestbase/jit/t/t104/t104.gold.
(win-x86_64, linux: ppc64, ppc64le, s390, x86_64, aarch, aix, solaris-sparc, mac)
Also, I modified quite some messages before which didn't cause any follow-up
breakages to my knowledge.

> I can't speak for the hotspot code, but that seems to add a lot of code
> to support
> only this convenience feature.  That's a maintenance overhead and burden.
We uses this code since 2006, it needed only few changes.  I would like to 
contribute a follow up for hidden frames of lambdas.

> The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
> question
> about how useful the information is,  developers should not need to know
> about "slot 1". 
Developers should work with class files with debug information and then
they would get the name printed. If exceptions are thrown in production
code, any information is helpful.  Should I just write "a local"?

> The test output of NullPointerExceptionTest shows
> "java.lang.Object.hashCode(()I)".
> Why is the argument type for Integer shown as "()I";  that's not the
> conventional
> representation of a primitive int.
I already discussed this with David Holmes:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/058464.html
Other exceptions use the same format. I don't know of an utility in
hotspot to translate the format to Java source code syntax.  We 
implemented such an utility in our internal VM, though, and I would
like to contribute this, too, adapting all the exceptions. I would do this
in a follow-up.

> Generally, the explanations are too verbose.
> Method names and field names would be easier to recognize if they were
> quoted, as is done with 'this'.  
Fixed, although this is not common in exception messages. See the
messages in http://hg.openjdk.java.net/jdk/jdk/file/2178d300d6b3/test/hotspot/jtreg/runtime/exceptionMsgs/IllegalAccessError/IllegalAccessErrorTest.java,
line 57ff. Only the String of the name field is quoted, not any entities
declared in the code.
Similar http://hg.openjdk.java.net/jdk/jdk/file/2178d300d6b3/test/hotspot/jtreg/runtime/LoaderConstraints/vtableAME/Test.java
line 60, also showing the internal signature, see above.

> Argument numbers should be digits,
> not English words (first, second, etc) to make them easier to pick out.
Fixed.
> And has it limits that do not read easily, e.g. "nr. 9".
Sorry, I don't understand
> I would not describe 'this' as a local variable.
Fixed.

> Tests, especially new tests should compile without warnings.
> NullPointerExceptionTest.java:125 newInstance has been deprecated.
Fixed.

> The messages can be easier to understand, e.g.
> 'field a1 is null, trying to invoke a1.hashCode...' instead of:
> "while trying to invoke the method java.lang.Object.hashCode(()I) on a
> null reference loaded from local variable 'a1'"
The message is built along the structure of the bytecode. 
I'll try to change this and then will come up with a new webrev .

> It will easier to understand if it looks more like the code.
> BTW, what does this look like for fully optimized code?
You mean whether the bytecode is jitted? It's independent
of how the code is executed, interpreted or compiled.

> Does it bear any resemblance to the source code at all?
> Or does it have to be deoptimized to come up with a sensible source
> reference.
No, jitted methods must not be deoptimized.

> How much of this can be done in Java code with StackWalker and other
> java APIs?
> It would be a shame to add this much native code if there was a more robust
> way to implement it using APIs with more leverage.
StackWalker operates on the Java stack tracke, which does not contain the BCI
to my knowledge.  Also, I need to read the bytecodes. Are there APIs to access 
the bytecode as it resides in the metaspace, rewritten, constants resolved etc?
The code relies on this.

Best regards,
  Goetz.

> Regards, Roger
> 
> 
> On 02/08/2019 09:13 AM, Langer, Christoph wrote:
> > Hi Goetz,
> >
> >> ok, so here a new webrev just adding a setter for the field:
> >> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02
> >>
> >> ... and incorporating the other comments.
> > Looks better ��
> >
> > I have a few additions to
> src/java.base/share/classes/java/lang/NullPointerException.java:
> >
> > 28 import java.lang.reflect.Field;
> > 29
> > -> can be removed now.
> >
> > 91         synchronized (this) {
> > -> I think this is not needed here. The transient modifier for
> lazyComputeMessage already implies the lock on the Object, I think.
> >
> > 108         return extendedMessage;
> > -> I guess it would be more correct if you returned super.getMessage() here.
> >
> > Thanks
> > Christoph