[PATCH] D11186: [mips][mcjit] Calculate correct addend for R_MIPS_HI16 and R_MIPS_PCHI16 relocations.

[Vladimir Radosavljevic via llvm-commits]

vradosavljevic marked 3 inline comments as done.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1337-1339
@@ +1336,5 @@
+      int64_t Addend = Value.Addend + SignExtend32<16>(Opcode & 0x0000ffff);
+      SmallVector<std::pair<RelocationValueRef, RelocationEntry>, 8>::iterator
+          I = PendingRelocs.begin();
+      while (I != PendingRelocs.end()) {
+        const RelocationValueRef &MatchingValue = I->first;
----------------
dsanders wrote:
> You could use a range-based for loop here at the moment. Unfortunately, fixing the comment above will prevent this. It should still use a for-loop instead of a while though.
I think that I can't use a range-based for loop, because I'm removing elements from SmallVector.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1342-1344
@@ +1341,5 @@
+        RelocationEntry &Reloc = I->second;
+        if (MatchingValue == Value &&
+            RelType == getMatchingLoRelocation(Reloc.RelType) &&
+            SectionID == Reloc.SectionID) {
+          Reloc.Addend += Addend;
----------------
dsanders wrote:
> mpf wrote:
> > dsanders wrote:
> > > This is almost certainly for a later patch but I'll mention it now. Does '.' work? For example:
> > > 
> > >   lui $2, $3, %hi(. + 4)
> > >   addiu $2, $3, %lo(. + 4)
> > > 
> > > I'm thinking it won't work in the current code because '.' drops unique private labels each time it's used. There's also an interesting special case with '.' and relocs like R_MIPS_(HI|LO)16. The '.' refers to the address for the %hi and the distance between the %hi and %lo is added to the addend. This is quite useful but it surprised me when I tried it. I previously thought the '.' was the current position and the programmer had to manually account for the distance.
> > Your description is wrong here. dot (.) always means instruction of the current instruction. You are confusing how this is then processed into a relocation. For pre-mips32r6 binutils will make the reference section relative which means that in a small test case it will look like dot refers to the first instruction when it is translated into a reference to the start of the section and an addend to hit the right instruction. You do need to manually account for the distance between the two instructions. Take a look at the relocations generated for mips32r6 for a clearer understanding as the relocs will not be made section relative there.
> Ah, I see my mistake(s) here. At the moment, our IAS doesn't convert the relocations to be section-relative so I'm used to seeing it symbol-relative and not section-relative. We need to fix that but it doesn't seem to do any harm so it's fairly low priority compared to the other issues. My test case also happened to be badly chosen with the %hi at offset zero from the start of the section. As a result the distance happened to be the same as the section-relative offset.
> 
> It's good to know that my original understanding was correct. Thanks
Is there anything that I need to do here?


Repository:
  rL LLVM

http://reviews.llvm.org/D11186