This is an archive of the discontinued LLVM Phabricator instance.

Implement some missing relocs for X86_64
AbandonedPublic

Authored by davide on Nov 12 2014, 2:19 PM.

Details

Summary

I'm still unsure about a couple of things:

  1. http://www.x86-64.org/documentation/abi.pdf page 72 mentions some of these relocations are not conformant to the ABI, so I'm not sure if these should be disallowed entirely?
  2. how to test this/where to put the tests

Diff Detail

Event Timeline

davide updated this revision to Diff 16114.Nov 12 2014, 2:19 PM
davide retitled this revision from to Implement some missing relocs for X86_64.
davide updated this object.
davide edited the test plan for this revision. (Show Details)
davide added reviewers: rafaelauler, shankarke.
davide added a subscriber: Unknown Object (MLST).
rafaelauler edited edge metadata.Nov 12 2014, 2:26 PM

Hi Davide,

Thanks for submitting your work. In LLD, we use the LLVM LIT test framework. To read more about it, visit http://llvm.org/docs/CommandGuide/lit.html. To see how this is used in the context of LLD, check the folder lld/test/elf. Each file in this folder is a different test. Start reading those files to get the hang of it.

As a general rule, avoid uploading binary files as part of tests. I know this is hard to do when testing a linker, because its inputs are binary files, but you can avoid this with some clever LLVM tools called obj2yaml and yaml2obj. These tools convert back and forth between binary files (including ELF) and a YAML description. Therefore, write your inputs in YAML then use yaml2obj to create the binary input, then ask lld to process it and see whether your relocations get resolved by converting the output to text again via obj2yaml.

Another thing, when submitting patches to Phabricator, it is interesting to provide patches with full context (see http://llvm.org/docs/Phabricator.html ). To do this, use

git diff -U999999 other-branch

-or-

svn diff --diff-cmd=diff -x -U999999

This eases the task of code review because we can see the entire code. It is not cluttered because Phabricator itself will collapse unchanged parts, but we can expand them on demand.

shankarke edited edge metadata.Nov 12 2014, 3:10 PM

Testcase ?

In D6235#6, @shankarke wrote:

Testcase ?

I'll write one for R_X86_64_PC64, but what about the others? Should I write one also for the relocations not supported in the ABI? Or the code should be changed to not allow those relocations?

+rafael

Does LLVM generate these "non-ABI-conformant" relocations? Do they need to
be supported by LLD? The relocations are R_X86_64_8, R_X86_64_16,
R_X86_64_PC16 and R_X86_64_PC8.

+rafael

Does LLVM generate these "non-ABI-conformant" relocations? Do they need to
be supported by LLD? The relocations are R_X86_64_8, R_X86_64_16,
R_X86_64_PC16 and R_X86_64_PC8.

I've rarely seen them in the wild but if you've already code the code then handling them isn't a problem.

-eric

emaste added a subscriber: emaste.Nov 12 2014, 4:45 PM
davide abandoned this revision.Feb 22 2015, 4:11 PM

Shankar already commited party of this indepentently, I'm gonna split this patch and add test case and resubmit for review.