This is an archive of the discontinued LLVM Phabricator instance.

[lld] [COFF, ARM64] Handle ADRP immediate offsets in relocations
ClosedPublic

Authored by mstorsjo on Jul 24 2017, 4:08 AM.

Details

Summary

Also handle overflow correctly in LDR/STR relocations. Even if the offset range of a 8 byte LDR instruction is 15 bit (even if the immediate itself is 12 bit) due to a 3 bit shift, only include up to 12 bits of byte level offset after doing the relocation, by limiting the range of the immediate by the number of shifted bits.

Diff Detail

Event Timeline

mstorsjo created this revision.Jul 24 2017, 4:08 AM
ruiu edited edge metadata.Jul 24 2017, 10:43 AM

It's not very clear to me if what this patch does is correct, even though I clearly understand each line of code. Can you write a function comment instead of writing a comment for each line?

mstorsjo updated this revision to Diff 107957.Jul 24 2017, 1:31 PM

Wrote a new per-function comment as suggested by Rui, hopefully explaining better what is done now.

ruiu added inline comments.Jul 24 2017, 2:58 PM
COFF/Chunks.cpp
204

So this is more like a scale than a size?

(Is there any documentation about this relocation?)

213–215

You are shifting Imm to the right and then to the left, which looks a bit odd.

215

So, if I understand correctly, this is equivalent to

write32le(Off, (Orig & FFB003FF) | (Imm + ...));

(Sorry I cannot calculate it in my head but I think it should be simpler.)

mstorsjo added inline comments.Jul 25 2017, 12:15 AM
COFF/Chunks.cpp
204

Yes, it's actually a shift/scale. I picked the name Size originally, because that's the name of the bits in the opcode, from the ARMv8 architecture reference manual.

I don't know of any documentation of the relocation itself - the official pecoff document only lists the relocation names/numbers, but no description of what they should do. This code is based on testing how link.exe handles them.

213–215

I can get reduce the amount of shifting by doing the calculation in scaled units instead of in bytes - I did that for clarity, but perhaps it only obscured things.

In that case, it's probably simplest to extend the existing applyArm64Imm with a parameter that limits the wraparound range/mask from 0xFFF to 0xFFF >> Scale - that's the actual effect of this change.

mstorsjo updated this revision to Diff 108037.Jul 25 2017, 3:18 AM
mstorsjo edited the summary of this revision. (Show Details)

Updated handling of scaled ldr relocations by using applyArm64Imm as before, but with a new parameter instead. Hopefully this is clearer.

ruiu added inline comments.Jul 25 2017, 12:01 PM
COFF/Chunks.cpp
187

We do not use optional parameters that often, as it basically defines two or more functions with the same name which increases program complexity (even though the increase may be small.) Just pass 0 instead.

test/COFF/arm64-symbol-offset.yaml
1

Can you reduce the size of the test? I don't think you need a test case of 200 lines to verify this change.

mstorsjo updated this revision to Diff 108147.Jul 25 2017, 1:31 PM

Removed use of a default parameter, squashed a smaller testcase into the existing arm64-relocs-imports.test instead.

mstorsjo marked 2 inline comments as done.Jul 25 2017, 1:31 PM

Rui, any further comments on this one?

ruiu accepted this revision.Jul 26 2017, 1:37 PM

LGTM

This revision is now accepted and ready to land.Jul 26 2017, 1:37 PM
This revision was automatically updated to reflect the committed changes.