Hi Goetz,
For NullPointerException, I don't think you want writeReplace() to call a
public method that can be overridden. You probably should create a private
helper method that both getMessage and writeReplace() call into.
Jason
________________________________________
From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of
Lindenmaier, Goetz <goetz.lindenma...@sap.com>
Sent: Tuesday, March 12, 2019 10:04 AM
To: Peter Levart; Roger Riggs; Java Core Libs
Cc: hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.
Hi Peter,
> 56 private static final String MESSAGE_PLACEHOLDER =
> "NPE_MESSAGE_PLACEHOLDER";
> Here, the initializer of that constant should create a private instance
> of String:
...
> private static final String MESSAGE_PLACEHOLDER = new String();
Ok, I understand.
> 81 public String getMessage() {
> 82 String message = super.getMessage();
> 83 if (message == MESSAGE_PLACEHOLDER) {
> 84 message = getExtendedNPEMessage();
> 85 setMessage(message);
> 86 }
> 87 return super.getMessage();
> 88 }
>
> Why not simply returning 'message' local variable here?
As I removed the synchronization, this would reduce the chance to
get different objects in case of a race. Highly unlikely though.
I changed the webrev with this solution to include these your remarks:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-PeterLevart/
(updated in-place).
Nevertheless, I think I will follow Roger's demand to not modify the
defaultMessage
field.
Best regards,
Goetz.
>
>
> Regards, Peter
>
> On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:
> > Hi Roger,
> >
> >> Maybe 10 years ago, when native was the only solution.
> >> Now there are tools to analyze bytecode in java.
> > I'm working on a Java implementation.
> >
> >>> Peter Levart proposed to initialize the message with a sentinel instead.
> >>> I'll implement this as a proposal.
> >> That's an extra overhead too, any assignment is.
> > Yes, any code requires some run time. But I think this really is negligible
> > in comparison to generating the backtrace, which happens on every
> > exception. But I'm fine with the 'always recompute, no serialization'
> > solution. If it turns out to be limited, it still can be changed easily.
> >
> >>> 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.
> >> Really!, if the bci is the same, the bytecode is the same, what could be
> different
> >> that would change the data used to create the exception?
> > e.getMessage().equals(e.getMessage()) will hold, but
> > e.getMessage() != e.getMessage().
> >
> > I just implemented the two variants of computing the message, they
> > basically differ in NullPointerException.java:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-
> PeterLevart/
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-
> RogerRiggs/
> >
> > I also cleaned up the parameters passed as proposed.
> >
> >>> We uses this code since 2006, it needed only few changes. I would like to
> >>> contribute a follow up for hidden frames of lambdas.
> >> Valhalla and evolution of the byte code needs to be considered.
> >> Just because its worked for years does not mean it's the best approach
> >> today. Dropping it in now means maintaining it in its current form
> >> or needing to re-write it later.
> > Well, yes, that is true. For any change. For any project. We have heard
> > this argument for many of our changes. I don't really think it's a good
> > argument. As I understand Valhalla is not targeted for jdk13, and
> > holding up changes for some future projects not really is the process
> > of OpenJDK, is it?
> >
> >>>> 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"?
> >> Go back to who you think is going to benefit from it, it seems targeted
> >> at a fairly novice developer, so exposing low level details may be more
> >> confusing that just giving a line number and NPE.
> > Especially on our improved NPE messages we always got good feedback
> > from the developers within SAP. And there are quite some experienced
> > people. Any message requires some background to be helpful. And I
> > like to get any information I can have in first place. Attaching the
> > debugger often is a big overhead. E.g., I look at about 50 failing jtreg
> > tests every day, I don't want to get each into the debugger every time
> > in the setup where it was running to see what is wrong ...
> >
> > E.g., com/sun/java/swing/plaf/windows/AltFocusIssueTest.java
> > is failing in a headless environment with an NPE on this line:
> > SwingUtilities.invokeAndWait(() -> frame.dispose());
> > With this change it said "while trying to invoke the method
> javax.swing.JFrame.dispose(()V) of a null object loaded from static field
> AltFocusIssueTest.frame" and I figured it's the fact we run headless that
> > makes the test fail without even looking at the code.
> >
> > Best regards,
> > Goetz
> >
> >
> >
> >
> >
> >
> >
> >
> > From: Roger Riggs <roger.ri...@oracle.com>
> > Sent: Dienstag, 12. Februar 2019 17:03
> > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Java Core Libs <core-
> libs-...@openjdk.java.net>
> > Cc: hotspot-runtime-...@openjdk.java.net
> > Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> >
> > Hi,
> > On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:
> > 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!!
> > Maybe, that's what debuggers are for.
> >
> >
> >
> > and is this the right level of implementation (C++/native)?
> > See below.
> > Maybe 10 years ago, when native was the only solution.
> > Now there are tools to analyze bytecode in java.
> >
> >
> >
> > 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.
> > That's an extra overhead too, any assignment is.
> >
> >
> >
> > 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.
> > Really!, if the bci is the same, the bytecode is the same, what could be
> different
> > that would change the data used to create the exception?
> >
> >
> >
> > 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.
> > Right, I realized this too when thinking about it over the weekend.
> >
> > If a suitable low impact mechanism can't be found, then just
> > go back to not exposing the extended message and use only the original.
> > Its a bad idea to twist and contort the local design and accessibility
> > just for serialization. What remote or delayed client needs to know this?
> >
> >
> >
> >
> > 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.
> > All of them are bad. Private needs to mean private.
> >
> > And making it mutable, also means that synchronization or volatile
> > is needed to ensure a consistent value for getMessage().
> > That turns into a performance issue for all throwables.
> > None of the above.
> >
> >
> >
> > That makes the writeReplace() unnecessary too.
> > No. I need this, see above.
> > See above, but is not essential to the core feature.
> >
> >
> >
> > 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.
> > It does seem unlikely.
> >
> >
> >
> > 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.
> > Valhalla and evolution of the byte code needs to be considered.
> > Just because its worked for years does not mean its the best approach
> > today. Dropping it in now means maintaining it in its current form
> > or needing to re-write it later.
> >
> >
> >
> > 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"?
> > Go back to who you think is going to benefit from it, it seems targeted
> > at a fairly novice developer, so exposing low level details may be more
> > confusing that just giving a line number and NPE.
> >
> >
> >
> > 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.
> > There may be better tools for formatting if it was done in java at a
> > more appropriate level of abstraction.
> >
> >
> >
> > 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/runt
> ime/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/runt
> ime/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
> > src/hotspot/share/classfile/bytecodeUtils.cpp:566
> >
> test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NullPointerE
> xceptionTest.java:612
> >
> > 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.
> >
> > From the other thread, beware adding overhead to the constructor.
> > The lazy computation is essential.
> > There are a *lot* of NPEs whose messages will never be needed.
> >
> > Regards, Roger
> >
> >
> >
> >
> > 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
> >
> >
