Hi,
I have been looking at how to implement this in Java using
StackWalker and ASM.
Either I oversee something or there is a row of deficiencies with
these tools to solve my issue.
StackWalker allows me to collect class name, method name, method
description and BCI. Given the current implementation, this must
happen in the NPE constructor which is very inefficient, but
works. Mandy, should I really change Throwable/StackWalker to
overcome this? Alternatively, I could JNI-call into the VM
and get the info when I need it.
ASM allows me to access the class and the bytecode of the method.
Unfortunately, ASM has no notion of BCI. It also does not
externally expose the bytecodes of the methods. It simplifies
the bytecodes, which is useful to manipulate them, but does
not allow me to recompute the BCI of instructions, say, iterating
the InsnList.
Further, as I understand, the bytecode I can access in ASM
is the bytecode as it resides in the class file, not the
bytecode used by hotspot (bitwise). Hotspot rewrites bytecodes when
it loads them. And the BCI returned by StackWalker corresponds
to the bytecode used by hotspot, not that in the class file.
If a method has been rewritten, there even might be several
instances of the same method/bytecode, the old still needed for
running executions and the new, rewritten one. The BCI
is only correct for one of these.
So I don't see how I should find the ASM representation of
the bytecode that caused the NPE. Do you have any advice?
Best regards,
Goetz.
> -----Original Message-----
> From: Peter Levart <peter.lev...@gmail.com>
> Sent: Freitag, 15. Februar 2019 18:38
> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Roger Riggs
> <roger.ri...@oracle.com>; Java Core Libs <core-libs-dev@openjdk.java.net>
> Cc: hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
>
> Hi Goetz,
>
> Just a couple of things if you go that route (of placeholder String):
>
>
> 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("NPE_MESSAGE_PLACEHOLDER");
>
> or even (since the content of the placeholder string doesn't matter -
> the identity does):
>
> private static final String MESSAGE_PLACEHOLDER = new String();
>
> ...or else anybody calling new
> NullPointerException("NPE_MESSAGE_PLACEHOLDER") will also be the subject
> of replacement since the string literals are interned constants. I.e.:
>
> "NPE_MESSAGE_PLACEHOLDER" == "NPE_MESSAGE_PLACEHOLDER"
>
>
> 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?
>
>
> 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
> >
> >
