RFR: 8163014: Mysterious/wrong value for "long" frame local variable on 64-bit

[Coleen Phillimore]

Max, this looks good.
Coleen

On 9/12/16 2:26 PM, dean.long at oracle.com wrote:
>
> On 9/12/16 9:06 AM, Max Ockner wrote:
>
>> New webrev: http://cr.openjdk.java.net/~mockner/8163014.hotspot.04/
>>  Comments below.
>> -Max
>> On 9/9/2016 1:54 PM, dean.long at oracle.com wrote:
>>>
>>>   614   movq(Address(rsp, 0), r);
>>> 615 movptr(Address(rsp, Interpreter::expr_offset_in_bytes(1)), 
>>> NULL_WORD );
>>> I wish these two looked more similar.  I guess movq is the same as 
>>> movprt, and
>>> Interpreter::expr_offset_in_bytes(0)
>>> could be used at 614?
>>
>> Yes. I have made this change.
> Looks good.
>
>>>
>>> 329 str(zr, pre(esp, -wordSize));
>>> 330 str(r, pre(esp, -wordsize));
>>> Do you want to use a single stp here?
>>>
>> The current patch is a working code suggestion from Andrew Dinn.  If 
>> it is important to change, I will see if I can build on aarch64 
>> before submitting.
> OK.  It's probably not important to change.
>
> dl
>
>>
>>> dl
>>>
>>> On 9/9/16 9:51 AM, Max Ockner wrote:
>>>> Oops. Here is version 2: 
>>>> http://cr.openjdk.java.net/~mockner/8163014.hotspot.03/
>>>>
>>>> Max
>>>>
>>>> On 9/8/2016 12:26 PM, Max Ockner wrote:
>>>>> Here is version 2 of this fix.
>>>>>  - Added copyright to LocalLongHelper.java
>>>>>  - Switched LocalLongTest.java to run with -Xint instead of 
>>>>> -Xcomp, and cleaned up some copypaste garbage.
>>>>>  - changed the test directory name from locallong to LocalLong.
>>>>>
>>>>> Reran test with -Xint.
>>>>>
>>>>> Thanks,
>>>>> Max
>>>>>
>>>>> On 9/7/2016 9:47 AM, Coleen Phillimore wrote:
>>>>>>
>>>>>> Max,
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mockner/8163014.01/test/runtime/locallong/LocalLongHelper.java.html 
>>>>>>
>>>>>>
>>>>>> There's a lot in this test, but I can't think of a different way 
>>>>>> to inspect the stack to find out if the long unused slot is 
>>>>>> zeroed. This file is missing a copyright, and I think the 
>>>>>> directory should be LocalLong (I think our new convention of test 
>>>>>> directory names is capitalized).  I think we should keep the test 
>>>>>> in the hotspot repository even though it uses the StackWalk API 
>>>>>> because otherwise it won't be run with hotspot changes on an 
>>>>>> ad-hoc or nightly basis.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mockner/8163014.01/test/runtime/locallong/LocalLongTest.java.html 
>>>>>>
>>>>>>
>>>>>> I thought the idea was to run this with -Xint not -Xcomp?
>>>>>>
>>>>>> The interpreter changes look good.
>>>>>>
>>>>>> Coleen
>>>>>>
>>>>>> On 9/6/16 5:33 PM, Max Ockner wrote:
>>>>>>> Hello,
>>>>>>> Please review this multi-platform fix for the stack walk API.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163014
>>>>>>> Webrev: http://cr.openjdk.java.net/~mockner/8163014.01/
>>>>>>>
>>>>>>> In 64 bits, long values can fit into a single slot, but two 
>>>>>>> slots are still used. The high slot contains garbage. This 
>>>>>>> normally wouldn't matter since it is never read from but the 
>>>>>>> stack walk API expects to display useful information. This is an 
>>>>>>> issue when displaying longs from local variables, so this means 
>>>>>>> we can kill any garbage off by zeroing it when it is pushed to 
>>>>>>> the stack in the previous frame. This solution zeroes the high 
>>>>>>> byte of a long value when it is being pushed to the stack (in 
>>>>>>> push_l).
>>>>>>>
>>>>>>> This applies to x86, aarch64, and sparc. This change does not 
>>>>>>> apply to ppc, though the bug almost certainly does affect it. I 
>>>>>>> have left ppc untouched since I don't have access to the 
>>>>>>> hardware required to reproduce the bug and test the fix.
>>>>>>>
>>>>>>> I have adapted the reproducer from the bug into a test. It is 
>>>>>>> curently sitting in runtime/locallong, but I believe there must 
>>>>>>> be a better place for it.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Max
>>>>>>
>>>>>
>>>>
>>>
>>
>