RFR(XS): 8234645: ARM32: C1: PatchingStub for field access: not enough bytes

[Doerr, Martin]

Hi Christoph,

thank you for testing it.

> One small remark:
> When I encountered the problem, I was very much confused by the constant
> instruction size of 8 [1]. Maybe it would be more clear to define the
> instruction size to be 4, and instead, in "num_bytes_to_end_of_patch()",
> return "instruction_size * 2"?

"instruction_size" refers to the size of a "NativeMovRegMem". It's used this way on other platforms, too.
I think the confusion comes from the fact that the "NativeMovRegMem" processor instructions get emitted individually on arm32, not by a "NativeMovRegMem" emitter which doesn't exist.
Emitting less than 8 bytes is always a bug (see set_offset which needs that space and you don't know in advance if the second 4 bytes will be written or not).

I need a 2nd review. I'll request an 11.0.6 backport afterwards.

Best regards,
Martin


> -----Original Message-----
> From: christoph.goettschkes at microdoc.com
> <christoph.goettschkes at microdoc.com>
> Sent: Mittwoch, 27. November 2019 10:09
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: hotspot-compiler-dev at openjdk.java.net
> Subject: Re: RFR(XS): 8234645: ARM32: C1: PatchingStub for field access: not
> enough bytes
> 
> Hi Martin,
> 
> thanks for looking into the issue.
> I executed the tier1 hotspot tests with your patch applied and it looks
> good. Tests which were failing before pass now. I have some, but unrelated
> failures, which I will start looking into now.
> 
> One small remark:
> When I encountered the problem, I was very much confused by the constant
> instruction size of 8 [1]. Maybe it would be more clear to define the
> instruction size to be 4, and instead, in "num_bytes_to_end_of_patch()",
> return "instruction_size * 2"?
> 
> Best regards,
> Christoph
> 
> [1]
> https://hg.openjdk.java.net/jdk/jdk/file/a2441ac23eeb/src/hotspot/cpu/ar
> m/nativeInst_arm_32.hpp
> 
> "Doerr, Martin" <martin.doerr at sap.com> wrote on 2019-11-26 17:21:58:
> 
> > From: "Doerr, Martin" <martin.doerr at sap.com>
> > To: "christoph.goettschkes at microdoc.com"
> > <christoph.goettschkes at microdoc.com>, "'hotspot-compiler-
> > dev at openjdk.java.net'" <hotspot-compiler-dev at openjdk.java.net>
> > Date: 2019-11-26 17:22
> > Subject: RFR(XS): 8234645: ARM32: C1: PatchingStub for field access:
> > not enough bytes
> >
> > Hi Christoph,
> >
> > thanks for reporting the bug
> > https://bugs.openjdk.java.net/browse/JDK-8234645
> >
> > Seems like the large offset fix in mem2reg and reg2mem missed the first
> > patching stub in the long/double cases.
> > We should have nop padding there, too (for same reason as for the 2nd
> > patching stub).
> > NativeMovRegMem should always consist of 2 instructions on arm32 in
> > order to support larger offsets.
> >
> > Webrev:
> > http://cr.openjdk.java.net/~mdoerr/8234645_arm_padding/webrev.00/
> >
> > May I ask you to test this fix? We don’t have arm32 in our testing
> landscape.
> >
> > Best regards,
> > Martin
> >