[PATCH v2] tidy the MCJIT tests now applyPermissions will invalidate cache

[Kaylor, Andrew]

Hi David,

The failure in the multiple_functions test problem points to something very ugly that I had forgotten about.  My guess is that the failure in that case is caused RuntimeDyldELF's failure to properly handle multiple applications of relocation information in the case of ARM (and also MIPS, if I recall correctly).  I think you could verify this by commenting out the call to Dyld.resolveRelocations in MCJIT::loadObject.  (It really shouldn't be there anyway.)

I'd prefer to have that test case disabled for ARM rather than have it use the alternate implementation.  The alternate implementation passes for the wrong reasons.  Even better would be to have the underlying problem fixed.

The thing that needs to change in RuntimeDyldELF to allow multiple relocations on ARM is to use Section.ObjAddress to read any values that need to be retrieved from the original object image.  For instance, the handler for R_ARM_ABS32 and R_ARM_TARGET1 currently looks like this:

  case ELF::R_ARM_TARGET1 :
  case ELF::R_ARM_ABS32 :
    *TargetPtr += Value;
    break;

(where TargetPtr = Section.Address + Offset).  You can see how that ends up with a wrong result the second time relocations are applied.

It should be doing something like this instead:

  case ELF::R_ARM_TARGET1 :
  case ELF::R_ARM_ABS32 :
    {
    uint32_t *Placeholder = reinterpret_cast<uint32_t*>(Section.ObjAddress + Offset);
    *TargetPtr = *Placeholder + Value;
    }
    break;

I don't know if it's more than you wanted to take on, but I'd be very happy to have that fixed.  There are a few ExecutionEngine/MCJIT tests marked as XFAIL: ARM because of this.

-Andy

From: David Tweed [mailto:david.tweed at arm.com]
Sent: Tuesday, May 14, 2013 9:00 AM
To: 'Filip Pizlo'
Cc: llvm-commits at cs.uiuc.edu; Kaylor, Andrew
Subject: RE: [PATCH v2] tidy the MCJIT tests now applyPermissions will invalidate cache

Hi,

| I'm totally with you on this!  And I agree with that being documented. I just think that calling finalizeObject() is the more canonical way of doing this, even if applyPermissions() also works - and the tests should do the canonical thing unless there is
|good reason not to.

|I do like the idea of adding more regression tests that cover even the non-canonical ways of doing things just so we can have a way of finding out if they break. But the generic tests should still do the canonical thing since they are not meant to cover the |more exotic behaviors.

Attached is a patch which modifies the existing tests (but doesn't add any new ones) to use finalizeObject() where possible. In testing on an ARM pandaboard I found that the multiple_functions test reliably segfaults if I used finalizeObject there. It also changes the name of the method from applyPermissions() to updateMemoryAttributes(), which I'd be happy with in another codebase but since "attribute" is a specific, different thing in LLVM I'm not sure it's a change for the better. My thesaurus didn't throw up any other likely candidates.

Any comments, or feedback from anyone testing on other architectures where this breaks the tests, is very welcome.

Cheers,
Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130514/c8310944/attachment.html>