On 3/26/19 4:57 AM, Lindenmaier, Goetz wrote:
and also the cases when it requires to look at its caller frame.
Never. Only the method that was executed when the exception is
thrown is needed. I only mention the backtrace because it happens to
store the top method and the corresponding bytecode position in the
top method.
That explains why I don't see an example of:
getNull().m();
public void m() {};
static NPE getNull() {
return null;
}
For a compiler expert or one who study the data flow analysis paper, is
this example very clear to them that it won't be supported?
This is related to my comment w.r.t. the data flow analysis comes from.
I was looking for some level of information be spelled out in the JEP
that can sufficiently give a non-compiler expert an idea but of course
no point to replicate the paper here.
It's good to know that this proposal needs the top frame only. Please
make it clear in
the JEP. The JEP can take out `StackWalker` but instead talks about
`StackFrame`
since doing it in Java means that it only needs to get back a
`StackFrame` instance
representing the top frame stored in `Throwable::backtrace`
This will avoid the confusion on calling StackWalker in the NPE
constructor and
constructing many StackFrame and many ResolvedMethodName. That was part
of the code review discussion.
"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.
(a ? b : c).d
You can not know whether the ?-expression yielded b or c, thus
you don't know why the access .d failed. There is a corresponding example
in [1].
Please describe in the JEP the known cases that this proposal doesn't
cover. Conditional expression is one and null receiver if it's the
return value from a caller frame (I think this would help the readers).
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.
Yes, note this also only addresses the method in which the exception was raised.
This should also go to the list of known cases that this proposal does
not cover.
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?
No, I can try to generate some numbers, though. But this assumption was made by
several others that commented on the previous review thread. For Throwable
in general, it is also the assumption why the intermediate backtrace data
structure is implemented instead of generating the stackFrames right away.
Since the JEP quotes that NullPointerExceptions are thrown frequently
and swallowed, it would help if you can generate some numbers.
This JEP will help the discussion on the proposed feature and design and
avoid any
confusion/assumption of the implementation.
"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.
Hmm, I don't completely understand what you want to say, please
excuse, I'm a foreign speaker :). I'll give a comprehensive answer:
To my understanding this is a consequence of the existing JVM/class file
implementation that calls methods that should be hidden to the user.
The implementation drops these frames when the backtrace datastructure
is computed. If this happens, the information needed to compute the
message is lost, and it can not be printed.
Yes I am familiar with that.
The fact that this happened must be remembered in the VM
so that printing the message can be suppressed. Else a message for
a completely wrong bytecode is printed.
Yup.
I think the JEP should mention that this is happening. How this is
implemented and then pushed to the repo is not subject to a JEP,
is it?
I think this is related to this JEP. The question would be:
- should it include all hidden frames?
- should it include the top frame if it's a hidden frame since this
feature only requires the top frame?
This JEP requires the hidden frame to be recorded in the backtrace. This
has
impact to the printing of stack trace that needs to know about hidden frames
and decide if it should be filtered or included when printing.
I made a webrev of its own for this problem, so that the code
can be looked at isolated of other, orthogonal issues.
http://cr.openjdk.java.net/~goetz/wr19/8221077-hidden_to_frame/
I don't need the webrev as we are discussing the proposed feature and
design.
The "Message persistance" section
I read this section like an implementation details
Yes, I had the feeling that I was talking too much about implementation
details,
but mostly in the "Basic algorithm" section.
This section discusses characteristics of NPE that differ from other exceptions
and that were proposed in the course of the review of my
original webrev. They are visible to the Java implementor.
So I think this is just the basic information needed in a JEP.
This is a good start that allows us to start the discussion. This JEP
will probably need several iterations of improvement. I'm writing a JEP
that I have iterated many times already and it makes the JEP and
proposed feature better. Just to share with you that it's part of the
process.
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.
Yes.
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.
It does not guarantee so, but it's the case for most exceptions. Usually,
one calls super(msg) in an Exception constructor.
It is a design decision to implement it that way, my original proposal
was different. I'm fine with this design, but it should be mentioned I think.
I think the JEP should be made clearer what you intend to say.
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.
I think implementing writeReplace() is a good idea, too. But
Roger does not like it:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/058513.html
The JEP should record this either if this is an open issue or makes it a
clear proposal to compute the message when NPE is serialized and what
the client would do (that may be running in a different version)
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.
Hmm, only printing local variable names is affected by the debug information.
So I don't think a section of its own is necessary.
If the matching bytecode is not available, nothing can be printed.
See my suggestion above to list all known cases that this proposed
feature will not cover, i.e. no improved message.
So it does not affect the content of the message. Should I call this
section "Message wording"? I think this is closer to what I want to
discuss in this paragraph.
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.
IAE quotes the names, which are user defined strings. It does not quote class
names, method names etc.
Here, I was asked to quote these, which is inconsistent with IAE. But there are
other
examples where method names are quoted ... So there is no clear line followed
yet. In the end, I don't care, but want advice how to do it. (I would not
quote ...).
You can add it to the open issues section.
"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.
The senctence above describes the status quo. The current implementation
of StackWalker must be called in the constructor.
I just called out the confusion this section could cause. I think it
should be updated
or rephase.
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).
Yes, obviously we can implement a lot of solutions. Instead of changing
StackWalker, we could simply expose [2]
java_lang_Throwable::get_method_and_bci()
to Java in NPE. After all, we only need the method which happens to be stored
as top frame in backtrace.
That's easy but implementation-detail.
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.
There is a unit test in the webrev. Please point out test cases you think
are missing. [1] stems from this test.
I am not commenting on [1] but the content of this section.
Please refer to the JEP template (http://openjdk.java.net/jeps/2) about
the Testing section.
|// What kinds of test development and execution will be required in
order // to validate this enhancement, beyond the usual mandatory unit
tests? // Be sure to list any special platform or hardware requirements. |
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.
I thought I picked out the most obvious example, pointer chasing, and used it
in the motivation and the description of the basic algorithm.
[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
Yes, I think it makes sense to put more messages into the JEP.
To be clear:
What I was suggesting is to describe the scenarios this proposed feature
will improve the message
but *not* to cut-n-paste the messages from [1]. So this is not tied to
what the message will be
printed from the implementation.
Writing the scenario in English can help discussing the NPE messages
should be implemented
that will be easy for Joe developer to understand.
Mandy
But I would
like to do that once it is agreed on the messages, so I don't need to
edit the JEP all the time.
The file referenced is simply generated by the test, so it's easy to update
and sure to reflect the implementation.
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.
Yes, sure, I can remove that.
Best regards,
Goetz.
