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

[Frederic Parain]

Thank you Coleen.

Fred

On 02/02/2015 16:41, Coleen Phillimore wrote:
>
> Frederic,
> This looks really good.
> Coleen
>
> On 1/29/15, 1:56 PM, Frederic Parain wrote:
>> Forgot new webrev:
>>
>> http://cr.openjdk.java.net/~fparain/8068655/webrev.02/
>>
>> Fred
>>
>> On 29/01/2015 19:53, Frederic Parain wrote:
>>> Dan,
>>>
>>> Thank you for your review.
>>>
>>> My answers are below.
>>>
>>> On 29/01/2015 19:24, Daniel D. Daugherty wrote:
>>>> On 1/29/15 8: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.
>>>>>
>>>>> 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/
>>>>
>>>> src/cpu/x86/vm/frame_x86.cpp
>>>>      nit: line 158: saved_fp = (intptr_t*)*(sender_sp -
>>>> frame::sender_sp_offset);
>>>>          Please add a space between the cast and the ptr deref, i.e.:
>>>>
>>>>          line 158: saved_fp = (intptr_t*) *(sender_sp -
>>>> frame::sender_sp_offset);
>>>
>>> Done
>>>
>>>>      line 214: frame sender(sender_sp, saved_fp, sender_pc);
>>>>          This line is not changed based on the assumption that
>>>>          (sender_sp == sender_unextended_sp). Do you want to
>>>>          assert() that just before line 214?
>>>
>>> After writing this assert it looked strange to me.
>>> I removed it and used the 4 arguments constructor to pass the
>>> unextended_sp computed line 154. It makes the code more regular
>>> between interpreted and not interpreted frames.
>>>
>>>>      line 580: if (fp() - unextended_sp() > 1024 + ...
>>>>          Previous to this line, the "stack pointer" checks all use
>>>> sp().
>>>>          A short comment explaining the switch to unextended_sp() would
>>>>          be good here.
>>>
>>> Done
>>>
>>>> Thumbs up. Your choice on whether to address my (very) minor
>>>> comments on this round.
>>>
>>> Thanks Dan,
>>>
>>> Fred
>>>
>>>>>
>>>>> 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