RFR(S)[13]: AArch64: float point register corruption in ZBarrierSetAssembler::load_at

Stuart Monteith stuart.monteith at linaro.org
Tue Jun 25 15:17:01 UTC 2019


Hello,
   I've updated the patch to just use rscratch1 and rscratch2, with
the appropriate asserts and register saving/restore as well as the
original fix. The call to load_mirror has been fixed to use r10
instead of expected rscratch1 to survive.

Webrev:
    http://cr.openjdk.java.net/~smonteith/8226515/webrev.1/

I'm looking at the follow on Item for allowing the temporary registers
passed to load_at "tmp1" and "tmp_thread" to be used, as currently
they are normally set to noreg.
Tested with jtreg tier1, JCStress, SpecJBB2015 with release and
fastdebug modes and G1GC, ParallelGC as well as ZGC.

BR,
   Stuart

On Mon, 24 Jun 2019 at 14:19, Stuart Monteith
<stuart.monteith at linaro.org> wrote:
>
> On Mon, 24 Jun 2019 at 11:07, Andrew Haley <aph at redhat.com> wrote:
> >
> > On 6/24/19 10:06 AM, Stuart Monteith wrote:
> > > Ok, r19 might have been a bad choice, but I'll work something out.
> > > rscratch1/2 might not be safe to use
> >
> > In the interpreter most of the lower-numbered registers are free. I'd
> > start at r10 and work up.
> >
> I'll give that a go.
>
> > > "load_at" is called by "access_load_at". If you look at
> > > templateTable_aarch64.cpp you see 25 instances of "noreg" being passed
> > > as tmp, 5 matches in methodHandles_aarch64.cpp. You'll find a similar
> > > story in arm and x86 and for store_at.
> >
> > Fair point. It's horrid. There's a great deal of bad practice copied
> > from x86, but you don't need to do this sort of thing in new code.
> >
> I'll test with not saving rscratch1,2 in ZGCs load_at and change
> generate_fixed_frame to not pass rscratch1 to load_mirror.
>
> > > I'd like to suggest I address the use of rscratch1/2 and the passing
> > > of noreg separately, the latter is a larger issue that should be
> > > handled in the original bug.  I can open a bug to handle the use of
> > > noreg, if that is appropriate.
> >
> > Sure.
>
> I'll start a fresh discussion on the mailing list - hotspot-dev would
> seem to be appropriate, and come to a consensus. I'm not sure whether
> at this point it is best to never pass temporary registers (i.e.
> remove them from the signatures), or we always pass temporary
> registers.
>
> >
> > I'm working on a patch which handles rscratch1 and 2 so that they are
> > declared at the point of use, not globally, and the compiler enforces
> > their allocation. However, it's a complicated job so won't happen
> > soon.  It also opens a can of worms.
>
> That would be nice - I'm always in favour of compilers enforcing rules
> - it is not always clear the context
> in which we'll be emitting code, and it does change. My first thought
> was to use RAII to enforce register usage, which
> might help solve one problem, but it is a deep rabbit hole.
>
> >
> > In new code, please reserve these registers for short-term scratch use
> > rather than as parameters or longer-term storage. That way I won't
> > have to fix things up later.
> >
> Yes, I've refactored out passing the scratch registers and depending
> on their results.
>
> > --
> > Andrew Haley  (he/him)
> > Java Platform Lead Engineer
> > Red Hat UK Ltd. <https://www.redhat.com>
> > https://keybase.io/andrewhaley
> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671



More information about the hotspot-gc-dev mailing list