[aarch64-port-dev ] icedtea7-aarch64 port changeset 5662 : make static stubs load methodOop in cpool to avoid problems at GC

[Andrew Haley]

On 10/27/2014 04:56 PM, Andrew Haley wrote:
> On 27/10/14 11:19, Andrew Dinn wrote:
>> Your advice and expertise is very welcome (even more so your patches :-)
> 
> Okay.  I'll have a look at it when I get a chance.  I don't
> like mysteries.

I've looked, and now I understand.

The patching machinery which is now in the back end was in shared code
for JDK7.  Unfortunately it's wrong for us: the code which changes the
immediate value in a compiled IC stub
(CompiledStaticCall::set_to_interpreted) does not set the OOP in the
associated reloc.  When the object referred to by that OOP is later
moved by a garbage collection, the immediate value in the stub is
trashed (with NULL).  As you said.

It works on x86 because the CompiledStaticCall uses a move immediate
instruction, and fetches and stores the target address directly in the
instruction stream.  The address in the reloc is not used at all.

It works on AArch64 JDK8 because there we use a metadata reloc (not an
oop reloc) and the code to adjust metadata relocs is null, I guess
because metadata isn't moved.

It works on AArch64 JDK7 if you use a non-immediate movoop because the
ldr instruction points straight into the OOP address in the reloc
table, and we patch it directly: therefore oop_Relocation::fix_oop_relocation()
does no more than reading from the reloc and writing it back to the
same place!  :-)

So, the code in JDK7 is wrong, which is why it doesn't work.  It can
be fixed, like this:

void CompiledStaticCall::set_to_interpreted(methodHandle callee, address entry) {
  address stub=find_stub();
  assert(stub!=NULL, "stub not found");

  RelocIterator iter((nmethod*)NULL, stub);
  oop_Relocation *reloc = NULL;
  while (iter.next()) {
    if (iter.addr() == stub) {
      switch(iter.type()) {
      case relocInfo::oop_type:
	{
	  reloc = iter.oop_reloc();
	  oop *o = (reloc->oop_addr());
	  *o = (oop)callee();
	}
	break;
      default:
	;
      }
    }
  }

I've tested this and it seems to work well.  But it's probably not
worth doing.  I'm happy that using a non-immediate movoop is OK.

You did the right thing, and I'm sorry I doubted you.

Phew.

Andrew.