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

[Ningsheng Jian]

Hi Max,

This patch breaks aarch64 build with:

error: ‘wordsize’ was not declared in this scope

I think it is a typo for "wordSize" at line below:

 330   str(r, pre(esp, -wordsize));

Thanks,
Ningsheng


On 13 September 2016 at 00:06, Max Ockner <max.ockner at oracle.com> 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.
>>
>>
>>   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.
>
>
>> 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
>>>>>
>>>>>
>>>>
>>>
>>
>