On 8/14/19 3:42 PM, Mandy Chung wrote:
I have further discussion with Coleen and walkthrough the vframe
implementation. Here is what we confirm and agree.
vframeStream::bci might return an invalid bci whereas javaVFrame::bci
returns a valid bci (compiledVFrame::bci adjusts invalid bci to 0).
An invalid bci could happen when hitting a safepoint on bci #0 on a
synchronized method either before or after the monitor is locked
(implicit synchronization without explicit monitorenter). That might
occur when StackOverflowError is thrown for example. However, that
should never happen when StackWalker is called and the top frame would
be in the stack walking code.
+1/-1 adjustment intends to address invalid bci case but such case is
not applicable for StackWalker API. With that, I agree with Coleen
that VM should set the bci value to StackFrameInfo::bci field and no
adjustment needed:
http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.05/
This looks good and straighforward.
Thank you!
Coleen
thanks
Mandy
On 8/13/19 4:56 PM, coleen.phillim...@oracle.com wrote:
Hi, I saw my name in this thread and had a discussion with Mandy. I
don't like that the VM and JDK having this special coordinated dance
of +1/-1, and the reason for this is to differentiate the value of 0
in StackFrame meaning either uninitialized or invalid. If through
some race, an unitialized value is observed, then the MemberName may
not be filled in either. In any case zero makes sense to return as
the bci because it'll point to the line number beginning of the
method, if used to get the line number.
The stackwalk API doesn't return invalid bci because it adjusts it:
int compiledVFrame::bci() const {
int raw = raw_bci();
return raw == SynchronizationEntryBCI ? 0 : raw;
}
At any rate, the 04 webrev looks good minus the +1/-1 dance and
StackWalker.java comment. Coupling the jdk and jvm like this feels
like it's asking for bugs. I like the simpler implementation that
fixes the bug that we have.
Thanks,
Coleen
On 8/13/19 1:49 PM, Mandy Chung wrote:
On 8/13/19 9:30 AM, Peter Levart wrote:
Usually the StackFrameInfo(s) are consumed as soon as they are
returned from StackWalker API and never assigned to @Stable field.
So there's no purpose of @Stable for bci field use. Except
documentation. But documentation can be specified in the form of
comments too.
There are several JDK classes whose fields are filled by VM and
comment is the form of documentation. Until new use of StackFrame
in the future shows performance benefit, it's okay to stick with
@Stable original intent and keep the comment:
+ private int bci; // zero and negative values
represent invalid BCI
Please also review CSR:
https://bugs.openjdk.java.net/browse/JDK-8229487
Mandy
