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

[Daniel D. Daugherty]

On 1/29/15 11:56 AM, Frederic Parain wrote:
> Forgot new webrev:
>
> http://cr.openjdk.java.net/~fparain/8068655/webrev.02/

Minor typo:

 > 579   // this test requires to use the unextended_sp which is the sp 
as seen by

to this:

 > 579   // this test requires the use of unextended_sp which is the sp 
as seen by

Thumbs up.

Dan


>
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>