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

[Lindenmaier, Goetz]

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