[PATCH] [mips] Resolve relocation for the stubs in MCJIT when load address is known

[Kaylor, Andrew]

Hi Petar,

In the case of ELF::R_X86_64_PC32, FinalAddress refers to the address that the relocation target will have when the generated code is executed, so calculating it based on Section.LoadAddress is correct.

Unless I have completely misunderstood your patch, the value you are calculating is the address of the stub and it should be possible to calculate that entirely based on Value and Addend.  If it isn't possible, I'd suspect that the relocation was created incorrectly.

The Value parameter comes from one of two places.  For relocations created using addRelocationForSection, it will always be the load address of the section specified when the relocation was added.  (This should always be the case for your stub handlers.)  For relocations created using addRelocationForSymbol, it will be the load address of the symbol.

Since you are redefining the meaning of R_MIPS_26 you should add very explicit comments saying so and explaining why.

I think you should be able to trigger the stub-already-defined code path by modifying one of the remote tests to call some function from multiple places.

-Andy

From: Petar Jovanovic [mailto:Petar.Jovanovic at imgtec.com]
Sent: Monday, November 18, 2013 3:40 PM
To: Kaylor, Andrew; llvm-commits at cs.uiuc.edu
Cc: NAKAMURA Takumi
Subject: RE: [PATCH] [mips] Resolve relocation for the stubs in MCJIT when load address is known

Hi Andy,

thanks for the response.
Relocation R_MIPS_26 in this particular case in runtime linker is used to hold information where the stub is (the relocation itself and addend are used here differently than in regular linker for the same relocation - this is offtopic, but just to say I am aware of that if anyone notices). The original relocation does not get called/resolved later (after processRelocationRef), so the idea was to emit the relocation, so it can be resolved once the final address is known.

In resolveMIPSRelocation(), Value will not always point to Section.LoadAddress at the function entry, thus an option is to use LoadAddress, this is similar how FinalAddress is calculated for ELF::R_X86_64_PC32.

Last, I am aware I skipped the part when stub is already generated. I can add in the patch similar change, e.g.

@@ -1031,8 +1033,9 @@ void RuntimeDyldELF::processRelocationRef(unsigned SectionID,
     //  Look up for existing stub.
     StubMap::const_iterator i = Stubs.find(Value);
     if (i != Stubs.end()) {
-      resolveRelocation(Section, Offset,
-                        (uint64_t)Section.Address + i->second, RelType, 0);
+      RelocationEntry RE(SectionID, Offset, RelType, i->second);
+      addRelocationForSection(RE, Value.SectionID);
+
       DEBUG(dbgs() << " Stub function found\n");
     } else {
       // Create a new stub function.


The reason why this was skipped in the first place was that none of the tests triggered that path, so I just left it untouched until I can find the case in which the change would be effective and correct.

Petar

________________________________
From: Kaylor, Andrew [andrew.kaylor at intel.com]
Sent: Monday, November 18, 2013 10:49 PM
To: Petar Jovanovic; llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Cc: NAKAMURA Takumi
Subject: RE: [PATCH] [mips] Resolve relocation for the stubs in MCJIT when load address is known
I don't think this patch is quite correct for two reasons.

First, in the call to resolveMIPSRelocation, the Section parameter refers to the section which contains the location to which the relocation is being applied while the Value parameter contains the LoadAddress of the symbol to which the relocation refers (in this case the section containing the stub).

It looks like the RuntimeDyldELF handling of this relocation guarantees the section containing the symbol to which the relocation refers will always be the same as the section which is the target of the relocation for the R_MIPS_26 relocation type, but in the general case (that is, across the ELF relocation handling) that isn't true.  The change you made to resolveMIPSRelocation will calculate the right value, I think, but I don't believe that change is even necessary.  Unless I'm mistaken, the line "Value = Section.LoadAddress + Addend" will not change the value of Value, which already contained the Value input argument (which should have been the section load address) + the Addend argument.  If this is true, the new change will just introduce potentially misleading code.

The second issue is more serious.  In RuntimeDyldELF::processRelocationRef, where you made your other change, there were two possible places where the old code would have attempted to apply the relocation immediately.  The place where you made your change handles the case where a new stub is created.  However, a few lines above that there is a place where the code found that the needed stub already existed and it attempts to immediately apply a relocation referencing that stub location.  I believe you also need to update that code.

-Andy


From: Petar Jovanovic [mailto:Petar.Jovanovic at imgtec.com]
Sent: Monday, November 18, 2013 12:46 PM
To: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Cc: Kaylor, Andrew; NAKAMURA Takumi
Subject: [PATCH] [mips] Resolve relocation for the stubs in MCJIT when load address is known

Hi everyone,

here is a patch for several MIPS MCJIT failures.

Regards,
Petar
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131119/0204f477/attachment.html>