This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF]: Minimal implementation for ARM static linking
ClosedPublic

Authored by denis-protivensky on Dec 17 2014, 11:11 PM.

Details

Summary

The skeleton code for ELF binary generation for ARM platform.

The code is able to statically link the simplest case of int main() { return 0; } sample with only ARM code (no Thumb code, no interwork, -marm, -mno-thumb-interwork options used) and with musl libc built with no interwork and no Thumb code as well.

Note: This is the replacement for http://reviews.llvm.org/D6446

Diff Detail

Event Timeline

denis-protivensky retitled this revision from to [lld][ELF]: Minimal implementation for ARM static linking.
denis-protivensky updated this object.
denis-protivensky edited the test plan for this revision. (Show Details)
denis-protivensky added a project: lld.
denis-protivensky added a subscriber: Unknown Object (MLST).
wnewton edited edge metadata.Jan 2 2015, 9:13 AM

This looks ok to me.

emaste added a subscriber: emaste.Jan 2 2015, 9:55 AM
shankarke accepted this revision.Jan 2 2015, 10:28 AM
shankarke edited edge metadata.
This revision is now accepted and ready to land.Jan 2 2015, 10:28 AM
ruiu added a subscriber: ruiu.Jan 12 2015, 1:12 PM
ruiu added inline comments.
lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
10

ifdef guard should include the path name.

#ifndef LLD_READER_WRITER_ELF_ARM_ARM_EXECUTABLE_WRITER_H
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.h
35

A name beginning with '_' followed by an uppercase character is reserved to the implementation (just like __), so please don't use such name. s/_ARMTargetLayout/_armTargetLayout/.

lib/ReaderWriter/ELF/ARM/ARMTarget.h
2

Why do we need this almost-empty header? Can't we directly include ARMLinkingContext.h?

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
63

They conflict with reserved names.

denis-protivensky edited edge metadata.

Updated:

  • header guard names reflect paths to files
  • member variables use lowercase letters right after dash symbol
ruiu accepted this revision.Jan 13 2015, 1:44 PM
ruiu added a reviewer: ruiu.

LGTM with this change.

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
40

_ARM -> _arm

denis-protivensky edited edge metadata.

Fixed variable spelling.

wnewton accepted this revision.Jan 14 2015, 1:54 AM
wnewton edited edge metadata.
atanasyan accepted this revision.Jan 14 2015, 2:58 AM
atanasyan edited edge metadata.

LGTM

I see everyone has accepted the revision.
How should I merge it as I don't have write permissions in the repo? Can someone help me with this please?

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
10

Good.

40

Fixed.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.h
35

Good, didn't know about that.

lib/ReaderWriter/ELF/ARM/ARMTarget.h
2

That's how the structure of files is organized.
Targets.h includes platform-specific *Target.h file, and it in turn includes its linking context.

ruiu added a comment.Jan 16 2015, 6:23 AM

Please upload your latest revision to Phabricator so that I can submit on
behalf of you.

denis-protivensky edited edge metadata.

Rebased the code to the actual sources.

Rui, please, merge the code. It's now up-to-date.

Hi Denis, I've volunteered to land this patch. Due to the size of the contribution, I'd like to put some extra effort into ensuring that your name ends up in the Author field on the git mirror. If we're able to configure git-svn to do that, I hope we can have it done and this patch landed in the next day or two. If you'd rather see it land today and don't mind my name stamped all over "git blame" output, I can go ahead and pull the trigger immediately. Please let me know your preference.

Also, the member variable _armTargetLayout is unused. If you make more changes before this lands, please delete that variable. Otherwise, I'll take care of it.

Thanks,
Greg

Hi Greg,

Thanks for taking care of it.
Concerning my name as an author: if it's possible, I'd like to keep my name and wait couple of days before you're done.
Concerning removing extra variable: I didn't plan to make any changes, but I can take care of it shortly.

Thanks,

Denis.

Removed unused variable.

Thanks for your patience. Could you please rebase your change on the latest? r226557 changed the TargetRelocationHandler and removed unhandledReferenceType().

Fixed after rebase:

  • TargetRelocationHandler ctor
  • make_unhandled_reloc_error() usage

Greg, updated. Thanks!

Thanks. I'm afraid we won't be able to reassign the author field in the git mirror until the next llvm.org update. I'll go ahead and land this patch now.

Committed in r226643. Thanks!

garious closed this revision.Jan 20 2015, 11:38 PM