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

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Jan 29 18:24:55 UTC 2015


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

     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?

     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.

Thumbs up. Your choice on whether to address my (very) minor
comments on this round.

Dan


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



More information about the hotspot-runtime-dev mailing list