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

[dean.long at oracle.com]

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