This is an archive of the discontinued LLVM Phabricator instance.

[COFF, ARM64] Add initial relocation types
ClosedPublic

Authored by mgrang on Jun 29 2017, 7:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang created this revision.Jun 29 2017, 7:10 PM
compnerd requested changes to this revision.Jun 29 2017, 10:48 PM

Please add tests for the other relocation types.

This revision now requires changes to proceed.Jun 29 2017, 10:48 PM
mgrang updated this revision to Diff 106788.Jul 15 2017, 2:46 PM
mgrang edited edge metadata.
mgrang retitled this revision from [COFF, ARM64] Add initial relocation types for COFF ARM64 target to [COFF, ARM64] Add initial relocation types.

Added more relocations.
Could not find a test case for IMAGE_REL_ARM64_ADDR32NB.
I see that IMAGE_REL_ARM64_SECTION is generated only on Windows with dumpbin (and not on Linux with llvm-objdump). Does it make sense to limit this unit test only for Windows?

rnk added inline comments.Jul 15 2017, 3:06 PM
test/MC/AArch64/coff-relocations.ll
1 ↗(On Diff #106788)

Use a .s test instead. You can probably run this file through llc, and then run llvm-mc on that to generate an object.

78 ↗(On Diff #106788)

With a .s test, you won't need all this complex debug info to make SECREL and SECTION relocations, you can just directly use the .secrel and .secidx directives.

@rnk Got it! Thanks. Will push an updated patch.

mgrang updated this revision to Diff 106795.EditedJul 16 2017, 12:07 AM

Changed unit test to .s according to comments.
Unit tests for IMAGE_REL_ARM64_PAGEOFFSET_12A and IMAGE_REL_ARM64_PAGEOFFSET_12L not added as llvm-mc currently cannot parse the assembly for these. Here is my code for the above two relocations:

adrp x0, foo
add x0, x0, foo

This is the error I get:

error: expected compatible register, symbol or integer in range [0, 4095]
add x0, x0, foo
                  ^

I see that IMAGE_REL_ARM64_SECTION is generated only on Windows with dumpbin (and not on Linux with llvm-objdump). Does it make sense to limit this unit test only for Windows?

I'd rather have a look at llvm-objdump and see if it can easily be fixed to show what you want.

Unit tests for IMAGE_REL_ARM64_PAGEOFFSET_12A and IMAGE_REL_ARM64_PAGEOFFSET_12L not added as llvm-mc currently cannot parse the assembly for these. Here is my code for the above two relocations:

adrp x0, foo
add x0, x0, foo

The correct syntax that llvm-mc should accept is this:

add x0, x0, :lo12:foo

IIRC this should also work with ldr/str:

ldr x0, [x0, :lo12:foo]
mgrang updated this revision to Diff 106809.Jul 16 2017, 10:17 AM

Added test cases for all the implemented relocation types.

I see that IMAGE_REL_ARM64_SECTION is generated only on Windows with dumpbin (and not on Linux with llvm-objdump). Does it make sense to limit this unit test only for Windows?

I'd rather have a look at llvm-objdump and see if it can easily be fixed to show what you want.

Unit tests for IMAGE_REL_ARM64_PAGEOFFSET_12A and IMAGE_REL_ARM64_PAGEOFFSET_12L not added as llvm-mc currently cannot parse the assembly for these. Here is my code for the above two relocations:

adrp x0, foo
add x0, x0, foo

The correct syntax that llvm-mc should accept is this:

add x0, x0, :lo12:foo

IIRC this should also work with ldr/str:

ldr x0, [x0, :lo12:foo]

Thanks @mstorsjo for pointing me to the correct assembly for add/ldr. My test cases now work for all the implemented relocation types.

compnerd accepted this revision.Jul 16 2017, 2:28 PM

Please fix the relocation type before you commit this.

lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp
50–74

Use auto and clang-format.

66

Shouldn't this be IMAGE_REL_ARM64_SECREL?

67

Why no newline here? Can you make the newline spacing uniform?

test/MC/AArch64/coff-relocations.s
3

Don't think that you need the second RUN: here.

This revision is now accepted and ready to land.Jul 16 2017, 2:28 PM
mgrang updated this revision to Diff 106823.Jul 16 2017, 4:58 PM

Addressed comments.

mgrang marked 3 inline comments as done.Jul 16 2017, 5:01 PM

I have addressed comments, and my previous patch was accepted by @compnerd. So unless someone has any more comments I will go ahead and commit this.

test/MC/AArch64/coff-relocations.s
3

Without the second RUN, this line is reported as malformed. I checked that a lot of the other tests also use a similar format of multiple RUNs.

This revision was automatically updated to reflect the committed changes.