[aarch64-port-dev ] RFR: 8163014: Mysterious/wrong value for "long" frame local variable on 64-bit

[David Holmes]

Thanks for that report. I have filed:

https://bugs.openjdk.java.net/browse/JDK-8166433

David

On 21/09/2016 2:45 PM, Ningsheng Jian wrote:
> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>