RFR(s): 8187547: PPC64: icache invalidation is incorrect in some places

[Anton Kozlov]

Hi, Goetz,

thanks for rewiew! 

I made suggested changes in the patch and preparied webrev for jdk10-master

http://cr.openjdk.java.net/~akozlov/8187547/webrev.03/

> I assume you are allowed to contribute to openJdk being with Azul who
> have signed the OCA?

Yes, Azul signed OCA and I'm working here.

Thanks,
Anton

On 15.09.2017 13:27, Lindenmaier, Goetz wrote:
> Hi Anton, 
> 
> thanks for fixing this issue. Looks good. 
> 
> Nevertheless I would find it more readable if the patch_* functions
> would just return the address of the first instruction.
> The code in nativeInst_ppc then would read
> 
>     if (MacroAssembler::get_address_of_calculate_address_from_global_toc_at(addr, cb->content_begin()) !=
>         (address)data) {
>       address inst2_addr = addr;
>       const address inst1_addr =
>         MacroAssembler::patch_calculate_address_from_global_toc_at(inst2_addr, cb->content_begin(),
>                                                                    (address)data);
>       ICache::ppc64_flush_icache_bytes(inst1_addr, inst2_addr - inst1_addr + BytesPerInstWord);
>     }
> 
> which I can read much more easily.
> Similar for patch_set_narrow_oop.
> You could add 
> // Returns address of first instruction in sequence.
> in macroAssembler_ppc.hpp.
> 
> Please update the Oracle copyright in nativeInst_ppc.cpp.
> 
> Also, jdk10/master is available.  Could you please prepare the webrev against 
> that repo?
>
> I assume you are allowed to contribute to openJdk being with Azul who
> have signed the OCA?
> 
> Best regards,
>   Goetz.
> 
> 
> 
> 
>> -----Original Message-----
>> From: ppc-aix-port-dev [mailto:ppc-aix-port-dev-
>> bounces at openjdk.java.net] On Behalf Of Anton Kozlov
>> Sent: Donnerstag, 14. September 2017 20:26
>> To: Doerr, Martin <martin.doerr at sap.com>; ppc-aix-port-
>> dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(s): 8187547: PPC64: icache invalidation is incorrect in some
>> places
>>
>> Martin,
>>
>> thanks for review!
>>
>> I tried to keep knowledge of where relocation is pointing to in
>> macroAssembler.
>> But if simplier code is preferable, of course I'm agree with this.
>>
>> Webrev with suggestion applied:
>> http://cr.openjdk.java.net/~akozlov/8187547/webrev.02
>>
>> I'm not very confident with the process for now (when repo closed), so
>> please do what should be done, I'll try to assist as much as I can.
>>
>> Backport should be easy, as the patch applies to jdk8u as well
>>
>> Thanks,
>> Anton
>>
>> On 14.09.2017 18:35, Doerr, Martin wrote:
>>> Hi Anton,
>>>
>>> thank you very much for providing a fix. Looks correct.
>>>
>>> In your current version, other_insn_offset is always negative.
>>> I'd prefer to make it always positive and simplify the usage like:
>>> assert(other_insn_offset > 0, "first instruction must be found");
>>> start = addr - other_insn_offset;
>>> range = BytesPerInstWord + other_insn_offset;
>>> This would be better readable. Would you agree?
>>>
>>> At the moment, jdk10 repos are temporarily closed, but we can sponsor
>> the change when it's open again and after a 2nd review.
>>> Backports will also need to get addressed.
>>>
>>> Please note that ppc-aix-port-dev is not appropriate for reviews because
>> the PPC64 port is part of the main repos. Therefore, I've added hotspot-
>> runtime-dev.
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: ppc-aix-port-dev [mailto:ppc-aix-port-dev-
>> bounces at openjdk.java.net] On Behalf Of Anton Kozlov
>>> Sent: Donnerstag, 14. September 2017 15:06
>>> To: ppc-aix-port-dev at openjdk.java.net
>>> Subject: RFR(s): 8187547: PPC64: icache invalidation is incorrect in some
>> places
>>>
>>> Hi, All!
>>>
>>> Icache invalidation range calculation in NativeMovConstReg::set_data_plain
>> and NativeMovConstReg::set_narrow_oop is incorrect and could cause VM
>> crash:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8187547
>>>
>>> I suppose the root is in mismatch of supposed and actual return values of
>> MacroAssembler::patch_set_narrow_oop and
>> MacroAssembler::patch_calculate_address_from_global_toc_at.
>>> These functions takes address of the middle of sequence and expected to
>> return first instruction offset (negative by current implementation). Instead
>> of this, they return `-offset == abs(offset)` and offset to `data` respectively.
>>>
>>> Supposed fix: http://cr.openjdk.java.net/~akozlov/8187547/webrev.01/
>>>
>>> Thanks,
>>> Anton
>>>