Advice on how to push changes back for ARMv7-A softfp platforms

[Boris Ulasevich]

Hi Christoph,

   Ok. I think you can use jtreg at least for regression testing. No 
need to pass it all, just check that your change does not make new fails.

   I would proceed with your first mail replying to it with usual 
Subject (RFR: 8229352: Use of an uninitialised register in 32-bit ARM 
template interpreter) and content including links to webrev and bugdb 
and description.

thanks,
Boris

On 09.08.2019 17:05, christoph.goettschkes at microdoc.com wrote:
> Hi Boris,
> 
> No, I did not run JTreg, the OpenJDK 11 is not completely ported to the
> target platform. Since it is a linux based system which does not use the
> glibc, but a different libc implementation, we only have a subset of the
> java modules ported to the target system. We used the OpenJDK JCK to
> verify the compliance of the created Java runtime. Since not all modules
> have been ported, we only tested the following modules:
> 
> - java.base
> - java.logging
> - java.naming
> 
> In addition, we executed the "vm" and "lang" test. The JCK went fine (all
> for the OpenJDK 11 code base). Since JTreg requires more modules to be
> ported, I might need make the port more complete. Are there any
> requirements of which parts of the jtreg test suite needs to be passed on
> the target platform? Porting all java modules might be a major task and we
> might not want to do that, since the change is inside HotSpot and we
> already verified the VM using the JCK.
> 
> How should I proceed with getting the change reviewed someone. Just keep
> this conversation open, or should I post an additional message with the
> bug ID and the webrev link and ask for a review?
> 
> Thanks,
> Christoph
> 
> Boris Ulasevich <boris.ulasevich at bell-sw.com> wrote on 2019-08-09
> 15:50:34:
> 
>> From: Boris Ulasevich <boris.ulasevich at bell-sw.com>
>> To: christoph.goettschkes at microdoc.com
>> Cc: hotspot-dev at openjdk.java.net
>> Date: 2019-08-09 15:50
>> Subject: Re: Advice on how to push changes back for ARMv7-A softfp
> platforms
>>
>> Hi Christoph,
>>
>>     I see. I do not know how to write test for the issue too. By the way,
> 
>> current implementation can (1) make unnecessary call to interpreter
>> runtime and (2) can make wrong load when Rtemp is equal to
>> JVM_CONSTANT_Long by chance. So the change is critical for arm32-sflt.
>>
>>     Did you run jtreg tests?
>>
>>     Please proceed with review. I can update webrev and bug report and
>> help you with pushing the change when it is accepted (I am not a
> reviewer).
>>
>> regards,
>> Boris
>>
>> On 09.08.2019 15:49, christoph.goettschkes at microdoc.com wrote:
>>> Hi Boris,
>>>
>>> thank you for the webrev.
>>>
>>> I checked jdk/jdk and you are right, there are differences in the
> file,
>>> but not regarding this issue. Some of the differences are because the
>>> AArch64 code has been removed from the cpu/arm port in jdk/jdk. In
>>> jdk-updates/jdk11u, the cpu/arm source base also includes an AArch64
> port.
>>> The patch looks good to me.
>>>
>>> I did not create a test case for this issue, because I don't know if
> one
>>> is able to check if the template interpreter calls the interpreter
> runtime
>>> or not.
>>> Resolving this issue makes the template interpreter faster, because it
>>> does not call the interpreter runtime (using the condy_helper
> function),
>>> but is able to load in the constant in the generated code. A simple
> test
>>> case (which I used and checked using a debugger, back when I patched
> our
>>> source base) is something like the following (It is not the test I
> used,
>>> but should work as well):
>>>
>>> class Test {
>>>       public static void main(String... args) {
>>>           long x = 12345678910L;
>>>       }
>>> }
>>>
>>> Also, since the Rtemp is not initialized in the code generated by this
>>> template, it could be, that the path for loading a long constant is
> used,
>>> even if the type is of double, but I don't know if this is only a
>>> theoretical bug or if it might appear in practice as well. I never saw
> it
>>> happening.
>>>
>>> Yes, there is an OCA signed for MicroDoc Software GmbH.
>>>
>>> Thanks, Christoph
>>>
>>> Boris Ulasevich <boris.ulasevich at bell-sw.com> wrote on 2019-08-09
>>> 13:49:00:
>>>
>>>> From: Boris Ulasevich <boris.ulasevich at bell-sw.com>
>>>> To: christoph.goettschkes at microdoc.com
>>>> Cc: hotspot-dev at openjdk.java.net
>>>> Date: 2019-08-09 13:49
>>>> Subject: Re: Advice on how to push changes back for ARMv7-A softfp
>>> platforms
>>>>
>>>> Hi Christoph,
>>>>
>>>> I can create webrev for you. Please note that we should not directly
>>>> patch jdk11u. We should push the fix to jdk-jdk, and then backport
> the
>>>> change to jdk11u. Your patch is not applied cleanly to the upstream
>>>> because of a minor change around exit label. I think for jdk-jdk the
>>>> change can be like this:
>>>> http://cr.openjdk.java.net/~bulasevich/8229352/webrev.00
>>>>
>>>> A good practice for review is to add a test to reproduce the problem.
>>>>
>>>> Did your company (or you personally) signed OCA?
>>>>
>>>> thanks,
>>>> Boris
>>>>
>>>
>>
>