6880034: Change 6420645 causes SIGBUS during deoptimisation at a safepoint on 64bit-SPARC

[Volker Simonis]

Hi Vladimir, Tom,

thank you for your evaluation.

You can find a webrev for this change at:

http://www.progdoc.de/webrev/6880034/

Could you please be so kind to review and submit it in case it's ok?

Thanks,
Volker


On 9/30/09, Vladimir Kozlov <Vladimir.Kozlov at sun.com> wrote:
> Volker Simonis wrote:
>  > I don't know why this strange encoding has been chosen for the 16
>  > upper double registers in sparc.ad, but changing it to:
>  >
>  > reg_def R_D32x(SOC, SOC, Op_RegD,255, F32->as_VMReg()->next());
>  > reg_def R_D32 (SOC, SOC, Op_RegD,  1, F32->as_VMReg());
>  > reg_def R_D34x(SOC, SOC, Op_RegD,255, F34->as_VMReg()->next());
>  > reg_def R_D34 (SOC, SOC, Op_RegD,  3, F34->as_VMReg());
>  > ...
>  > reg_def R_D62x(SOC, SOC, Op_RegD,255, F62->as_VMReg()->next());
>  > reg_def R_D62 (SOC, SOC, Op_RegD, 31, F62->as_VMReg());
>  >
>  > which seems more natural to me, solved the SIGBUS issue and didn't
>  > revealed any other problems in the tests which I run so far.
>  >
>  > Could you please comment on the proposed solution of changing the
>  > VMReg numbering of F32-F62 or advice a better solution if you think
>  > that the proposed one will not work in the general case?
>  >
>  > Thank you and best regards,
>  > Volker
>
>  Your changes are correct but I would also swap definitions
>  to be more clear.
>
>  reg_def R_D32 (SOC, SOC, Op_RegD,  1, F32->as_VMReg());
>  reg_def R_D32x(SOC, SOC, Op_RegD,255, F32->as_VMReg()->next());
>  reg_def R_D34 (SOC, SOC, Op_RegD,  3, F34->as_VMReg());
>  reg_def R_D34x(SOC, SOC, Op_RegD,255, F34->as_VMReg()->next());
>
>  I looked on the history and originally it was
>
>  reg_def R_D32( SOC, SOC, Op_RegD,  1, 0 );
>  reg_def R_D32L(SOC, SOC, Op_RegD, 99, 0 );
>  reg_def R_D34( SOC, SOC, Op_RegD,  3, 0 );
>  reg_def R_D34L(SOC, SOC, Op_RegD, 99, 0 );
>
>  where R_D32L and R_D34L represented nonexisting F33 and F35
>  low 32 bits halves of D32 and D34.
>
>  Then the ordering of declarations was changed and introduced
>  the confusion:
>
>  reg_def R_D32x(SOC, SOC, Op_RegD,255, 0 );
>  reg_def R_D32 (SOC, SOC, Op_RegD,  1, 0 );
>  reg_def R_D34x(SOC, SOC, Op_RegD,255, 0 );
>  reg_def R_D34 (SOC, SOC, Op_RegD,  3, 0 );
>
>  Vladimir
>
>  Tom Rodriguez wrote:
>
> >
> > > The problem can be easily solved by switching back to the old
> > > implementation of frame::oopmapreg_to_location(), but I
> assume there
> > > was a rational behind the change and that the new implementation is
> > > probably necessary for compressed oops (at least that's what the whole
> > > change was all about). So I dug a little further and found that in my
> > > opinion the root cause of the whole problem is the strange numbering
> > > of the 16 upper double registers in sparc.ad. They are defined as
> > > follows:
> > >
> > > reg_def R_D32x(SOC, SOC, Op_RegD,255, F32->as_VMReg());
> > > reg_def R_D32 (SOC, SOC, Op_RegD,  1, F32->as_VMReg()->next());
> > > reg_def R_D34x(SOC, SOC, Op_RegD,255, F34->as_VMReg());
> > > reg_def R_D34 (SOC, SOC, Op_RegD,  3, F34->as_VMReg()->next());
> > > ...
> > > reg_def R_D62x(SOC, SOC, Op_RegD,255, F62->as_VMReg());
> > > reg_def R_D62 (SOC, SOC, Op_RegD, 31, F62->as_VMReg()->next());
> > >
> >
> > I don't really know why it's done this way.  It's certainly not consistent
> with the others.  If it all works better I'd be ok with correcting.
> >
> >
> > > PS: while I was hunting the error, I also stumbled across the
> > > following code in RegisterSaver::save_live_registers():
> > >
> > >  // Save all the FP registers
> > >  int offset = d00_offset;
> > >  for( int i=0; i<64; i+=2 ) {
> > >   FloatRegister f = as_FloatRegister(i);
> > >   __ stf(FloatRegisterImpl::D,  f, SP, offset+STACK_BIAS);
> > >
> map->set_callee_saved(VMRegImpl::stack2reg(offset>>2),
> f->as_VMReg());
> > >   if (true) {
> > >     map->set_callee_saved(VMRegImpl::stack2reg((offset
> +
> > > sizeof(float))>>2), f->as_VMReg()->next());
> > >   }
> > >   offset += sizeof(double);
> > >  }
> > >
> >
> > That would probably be ok too.
> >
> > tom
> >
> >
> > >
> > > In my opinion, this could be changed to:
> > >
> > >  // Save all the FP registers
> > >  int offset = d00_offset;
> > >  for( int i=0; i<64; i+=2 ) {
> > >   FloatRegister f = as_FloatRegister(i);
> > >   __ stf(FloatRegisterImpl::D,  f, SP, offset+STACK_BIAS);
> > >
> map->set_callee_saved(VMRegImpl::stack2reg(offset>>2),
> f->as_VMReg());
> > >   if (i < 32) { // VS 2009-08-31: the 16 upper double registers
> > > can't be used as floats anyway
> > >     map->set_callee_saved(VMRegImpl::stack2reg((offset
> +
> > > sizeof(float))>>2), f->as_VMReg()->next());
> > >   }
> > >   offset += sizeof(double);
> > >  }
> > >
> > > because the 16 upper double registers can't be used as floats anyway.
> > > Again, I didn't found any regression in my few tests. What do you
> > > think?
> > > <DeoptTest.java>
> > >
> >
> >
>