RFR(XS): JDK-8068655 frame::safe_for_sender() computes incorrect sender_sp value for interpreted frames

[Frederic Parain]

On 01/30/2015 05:01 AM, David Holmes wrote:
> Hi Fred,
>
> On 30/01/2015 1:05 AM, Frederic Parain wrote:
>> Hi Dean,
>>
>> When unextended_sp is not specified, it is set to the sp value.
>>
>> However, after your question, I realized that my fix was not
>> correct but worked because the is_interpreted_frame_valid() had
>> a bug too, it uses sp() where unextended_sp() should be used.
>
> Just wondering what the implications of this bug would be? What code
> would be wrong, how would it manifest?

The effect of this bug is that is_interpreted_frame_valid() could
return false in some cases where the frame is valid. It impacts all
assertions relying on safe_for_sender() and also profilers and JFR
which are also relying on safe_for_sender to decide if it is safe
or not to walk the stack.

In many cases, the bug doesn't show up, but of the callee method
is interpreted and has a huge number of local variables (see
test in hotspot/test/compiler/uncommontrap/TestStackBangMonitorOwned.java)
then is_interpreted_frame_valid() returns an invalid result.

Regards,

Fred

>> Checking the history of these files, is_interpreted_frame_valid() has
>> been written before the distinction between raw sp and unextended sp
>> has been introduced in the frame class. Unfortunately, when
>> unextended_sp was added, is_interpreted_frame_valid() has not been
>> updated.
>>
>> Here's a new webrev where safe_for_sender computes both raw_sp and
>> unextended_sp, and is_interpreted_frame_valid() checks the frame
>> size using unextended_sp instead of raw sp.
>>
>> http://cr.openjdk.java.net/~fparain/8068655/webrev.01/
>>
>> I also added more comments.
>>
>> Note that I only modify one test in is_interpreted_frame_valid().
>> Use of sp() or unextended_sp() could be discussed for each test,
>> but in their current form they are wrong.
>>
>> Thanks,
>>
>> Fred
>>
>> On 26/01/2015 20:40, Dean Long wrote:
>>> In sender_for_interpreter_frame(), we set the raw sender_sp but also
>>> unextended_sp.  Should
>>> safe_for_sender() be doing the same?
>>>
>>> dl
>>>
>>> On 1/26/2015 6:40 AM, Frederic Parain wrote:
>>>> Greetings,
>>>>
>>>> Please review this small fix in the frame verification code.
>>>> The bug report includes a detailed description of the issue.
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8068655
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~fparain/8068655/webrev.00/
>>>>
>>>> Thanks,
>>>>
>>>> Fred
>>>>
>>>
>>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com