RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[coleen.phillimore at oracle.com]

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.phillimore at 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
>>
>