This is an archive of the discontinued LLVM Phabricator instance.

[mips][mcjit] Calculate correct addend for R_MIPS_HI16 and R_MIPS_PCHI16 relocations.
ClosedPublic

Authored by vradosavljevic on Jul 14 2015, 6:41 AM.

Details

Summary

So far, for O32 ABI we didn't calculate correct addend for R_MIPS_HI16 and R_MIPS_PCHI16 relocations. This patch fixes that.

Diff Detail

Repository
rL LLVM

Event Timeline

vradosavljevic retitled this revision from to [mips][mcjit] Calculate correct addend for R_MIPS_HI16 and R_MIPS_PCHI16 relocations..
vradosavljevic updated this object.
vradosavljevic added reviewers: dsanders, petarj.
vradosavljevic set the repository for this revision to rL LLVM.
vradosavljevic added a subscriber: llvm-commits.
dsanders added inline comments.Aug 11 2015, 9:27 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1334

Won't this get expensive when there's lots of relocations? SmallVectors is an array internally so insert() has to move the elements after the insertion point (i.e. all elements) to make room for the new one.

Can we use push_back() here and a backwards search below?

1337–1339

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.

1342–1344

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.

test/ExecutionEngine/RuntimeDyld/Mips/ELF_O32_PIC_relocations.s
64

Nit: indentation

mpf added a subscriber: mpf.Aug 11 2015, 3:10 PM
mpf added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1342–1344

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.

dsanders added inline comments.Aug 11 2015, 3:33 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1342–1344

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

vradosavljevic marked 3 inline comments as done.Aug 12 2015, 6:19 AM
vradosavljevic added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1337–1339

I think that I can't use a range-based for loop, because I'm removing elements from SmallVector.

1342–1344

Is there anything that I need to do here?

vradosavljevic marked an inline comment as done.

Comments addressed.

dsanders accepted this revision.Aug 12 2015, 8:32 AM
dsanders edited edge metadata.

LGTM.

We'll have to revise this to follow the rules more closely as the corner cases start to appear but being able to correctly process the majority of HI16/LO16 pairs is a big step forwards compared to what we had before

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1337–1339

I notice you're still iterating forwards here even though you now call push_back above. I was going to say that you need to reverse the iteration direction but having re-read the (very confusing) rules again, I think iterating forwards is the right thing to do.

The reason for this change of opinion is that the HI16's would normally be sunk down to their matching LO16 which is often the first LO16 with the same symbol after the HI16 (there's some corner cases where there are multiple candidates). By iterating forwards, this code effectively does the same thing except the LO16 searches for the matching HI16.

I think that I can't use a range-based for loop, because I'm removing elements from SmallVector.

For iterating forwards, I agree. If we iterated backwards, that would have been the reason instead.

1342–1344

Not for this patch.

For later patches:
I think the 'MatchingValue == Value' is stricter than it should be. The addend in the %lo is allowed to differ from one in the %hi on the condition that the user knows they didn't cause a carry from the lo16 to the hi16.

There's also the issue of '.' not working properly but that's a consequence of us not transforming relocations to be section-relative instead of symbol-relative. Once that's fixed, '.' will also work.

This revision is now accepted and ready to land.Aug 12 2015, 8:32 AM
This revision was automatically updated to reflect the committed changes.