Hi,
A few higher level issues should be considered, though the details of
the webrev
captured my immediate attention.
Is this the right feature and is this the right level of implementation
(C++/native)?
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 think there are ways to make this easier to implement and be easier to
maintain
and perform better.
NullPointerException:
28: unused import of Field
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.
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.
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().
121: Simplify getExtendedNPEMessage by making it only responsible for
the detail
and not concatenation with an existing message. Put that in
getMessage().
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).
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.
That makes the writeReplace() unnecessary too.
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.
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?
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.
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".
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.
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'. Argument numbers should be digits,
not English words (first, second, etc) to make them easier to pick out.
And has it limits that do not read easily, e.g. "nr. 9".
I would not describe 'this' as a local variable.
Tests, especially new tests should compile without warnings.
NullPointerExceptionTest.java:125 newInstance has been deprecated.
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'"
It will easier to understand if it looks more like the code.
BTW, what does this look like for fully optimized code?
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.
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.
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
