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

[Lindenmaier, Goetz]

HI Anton, 

I pushed the change the other day, but obviously I forgot to set you as author.
Sorry for that. Feel free to account it to you by referencing this email.

Best regards,
  Goetz



> -----Original Message-----
> From: Anton Kozlov [mailto:akozlov at azul.com]
> Sent: Dienstag, 19. September 2017 13:44
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 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
> 
> 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
> >>>