[llvm-objcopy] Add support for -I binary -B <arch>.

[rupprecht]

Summary

The -I (--input-target) and -B (--binary-architecture) flags exist but are currently silently ignored. This adds support for -I binary for architectures i386, x86-64 (and alias i386:x86-64), arm, and aarch64. This is largely based on D41687.

This is done by implementing an additional subclass of Reader, BinaryReader, which works by interpreting the input file as contents for .data field, sets up a synthetic header, and adds additional sections/symbols (e.g. _binary__tmp_data_txt_start). Additionally, change the symbol table to own symbols, by changing StringRef to std::string, as the synthetic symbols added are otherwise unowned.

[jakehehrlich]

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

tools/llvm-objcopy/Object.cpp

619 ↗

(On Diff #159343)

Correct me if I'm wrong but I don't think GNU objcopy produces any program headers when it does this. The resulting file should be relocatable and thus program headers are pointless. Even if GNU objcopy does this, we probably shouldn't.

652 ↗

(On Diff #159343)

The MemBuf should outlive the section in which case you can just use Section

656 ↗

(On Diff #159343)

nit: Can you use a Twine? You'll have to use an std::string for the buffer identifier part unfortunately but other than that you should be able to use a twine.

676 ↗

(On Diff #159343)

I think you're ok to initialize the symbol table in whatever order you want here since there are no relocation. That's a current thorn that you even have to initialize things in a different order. I need to carve out some time to refactor llvm-objcopy to work differently to get there however.

[rupprecht]

• Refactor Object based on code review comments

[rupprecht]

In D50343#1190050, @jakehehrlich wrote:

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

Yeah, it's not my first choice either, but it did make the implementation easier and seemed somewhat consistent with the pattern elsewhere in this file.

I wasn't sure what plans you had for refactoring things here (still haven't seen whatever thread was referenced earlier). My hunch is that "the right way" would be to not have any extra ELF type here and just always directly use what's in Object/ELFTypes.h, and to do things with composition instead of templated-inheritance, and I'm happy to make that change (or whatever alternative change makes sense), but that seems like an orthogonal discussion that isn't related to the features implemented here.

tools/llvm-objcopy/Object.cpp

619 ↗

(On Diff #159343)

I just verified this; commenting out this section causes an invalid object file (readelf/llvm-readobj can't read it at all).
This is ElfHdrSegment. I think you are confusing this with ProgramHdrSegment?

However, speaking of program headers, line 1290 (i.e. OrderedSegments.push_back(&Obj.ProgramHdrSegment)) seems to do nothing -- all the tests pass -- and also gets rid of the error readelf: Warning: possibly corrupt ELF header - it has a non-zero program header offset, but no program headers when running readelf -a on object files that llvm-objcopy produces. I'd like to take a look at that after this patch; it seems to be a preexisting issue.

[rupprecht]

• Pass MachineInfo into BinaryReader to push ELFType template down to only BinaryELFBuilder<>
• Move getElfType out of Reader (and also ELFReader), it can be implemented purely in the driver class. This make it clear that "Reader" is not tied to ELF types, even if that's all that's supported currently.

[rupprecht]

In D50343#1190161, @rupprecht wrote:

In D50343#1190050, @jakehehrlich wrote:

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

Yeah, it's not my first choice either, but it did make the implementation easier and seemed somewhat consistent with the pattern elsewhere in this file.

OK, I removed ELF templates from BinaryReader; having them remain on BinaryELFBuilder still seems appropriate though.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

592 ↗

(On Diff #159772)

So I this function is the real problem child. We don't actually use the Ident anywhere.This is also 100% comprised of information that the ELFWriter knows but nothing else knows. We should make the ELFWriter construct the Ident and just not store it in Object.

619 ↗

(On Diff #159343)

You're right. I confused it with ProgramHdrSegment. I now remember that someone had the clever idea to make the ELF header an implicit segment so that the same layout algorithm that had all the bugs worked out could already be used. As for the warning I guess I knew about that issue. The OrderedSegments.push_back(&Obj.ProgramHdrSegment) should accomplish adding that program headers to layout when you have a PT_PHDR segment in a consistent way. The warning is a separate issue on line 1078 (in this patch) not checking if the program headers are empty before blindly setting phoff to the offset of that segment. This is in full compliance with the ELF standard because the number of program headers is still zero. It's just that no tool normally has a reason to produce an ELF that has phoff != 0 and phnum = 0 so it's normally a sign of corruption.

[rupprecht]

tools/llvm-objcopy/Object.cpp

592 ↗

(On Diff #159772)

Nice, I was able to completely remove Ident from Object.

619 ↗

(On Diff #159343)

I tried adding that check, and several tests failed -- I might not have been checking the right thing, or maybe the tests are bad. I added a TODO on that line to investigate further.

[rupprecht]

• Remove Ident from object

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

619 ↗

(On Diff #159343)

How did the tests fail? Some of the tests do a literal check against the header. e.g. they may be checking to see that 0x40 is used even when (to comply with the warning) 0x0 should be used instead.

[rupprecht]

• Add sparc/ppc to arch map

[rupprecht]

tools/llvm-objcopy/Object.cpp

619 ↗

(On Diff #159343)

With this change:

// Obj.ProgramHdrSegment.firstSection() == nullptr implies
// Obj.ProgramHdrSegment.Sections is empty
Ehdr.e_phoff = Obj.ProgramHdrSegment.firstSection() == nullptr
                   ? 0
                   : Obj.ProgramHdrSegment.Offset;

The failures usually looked like:

Command Output (stderr):
--
<src>/llvm/test/tools/llvm-objcopy/triple-overlap.test:72:14: error: CHECK-NEXT: expected string not found in input
#CHECK-NEXT: Type: PT_LOAD (0x1)
             ^
<stdin>:9:2: note: scanning from here
 Type: (0x464C457F)
 ^
<stdin>:21:2: note: possible intended match here
 Type: (0x400004)
 ^

Actually, it looks like the object is corrupted with that change, e.g. the first program header for that test has an alignment of 15762873573703680, vs 4096 for all of them on the base side.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

619 ↗

(On Diff #159343)

Yeah that's not the right check. Use Obj.ProgramHeaders.size() == 0

[rupprecht]

tools/llvm-objcopy/Object.cpp

619 ↗

(On Diff #159343)

I can't seem to get that to work:

• Obj doesn't have a field called ProgramHeaders
• You probably mean Obj.ProgramHdrSegment, but Segment doesn't have a method size()
• If I implement size() as returning either Contents.size() or Sections.size() and then try:

Ehdr.e_phoff =
    Obj.ProgramHdrSegment.size() == 0 ? 0 : Obj.ProgramHdrSegment.Offset;

Then I get the same failure as before

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

607 ↗

(On Diff #159809)

I think ELFWriter should assign these. I think I need to do some refactoring to make Object not so ELF specific (with respect to not containing fields that are not known until write time). This change is exposing a lot of points where I failed to properly separate those concerns. In the mean time if you could move this code into ELFWriter and call it form assignOffset that would be ideal.

619 ↗

(On Diff #159343)

hmm...that's bothersome; I must not be understanding something. I'll see about looking into that. Thanks for letting me know about this!

[rupprecht]

• Extract out ehdr initialization into initEhdr()

[rupprecht]

tools/llvm-objcopy/Object.cpp

607 ↗

(On Diff #159809)

I didn't find a way to refactor out setting ElfHdr.Index, since ELFBuilder<ELFT>::readProgramHeaders() is setting it to ElfHdr.Index = Index++ in the middle of some other sections, but the rest was simple to refactor into ELFWriter.

[rupprecht]

• Consistently use ELFT typename aliases in headers

[jakehehrlich]

Awesome! I'd like to have someone else look over this but this pretty well LGTM.

tools/llvm-objcopy/Object.cpp

614 ↗

(On Diff #159941)

And this is the last thing where we can't easily factor out dependence on ElfType. This is going to require that Size be handled differently from how it is now. I'm not going to block this change on that since the fix is just not simple. Under the current design of llvm-objcopy that would require a new visitor to calculate the size. I'm not sure I want to further things along that path.

Can you add a TODO here for me?

[rupprecht]

• Add Elf_Sym TODO

[rupprecht]

Thanks for the review!

Alex/James/anyone else who's lurking, mind taking a look for a second set of eyes?

[jhenderson]

In D50343#1194094, @rupprecht wrote:

Thanks for the review!

Alex/James/anyone else who's lurking, mind taking a look for a second set of eyes?

Yup, just going to look now. Sorry, it's been a busy couple of days, otherwise I'd have done so earlier.

[jhenderson]

test/tools/llvm-objcopy/binary-input-aarch64.test

1 ↗

(On Diff #159961)

Use echo, not printf. I'm not sure if printf is implemented in the lit test system etc, but echo is the more common approach anyway.

3 ↗

(On Diff #159961)

You dump the symbols here, but don't check any of them... you should probably do so!

8 ↗

(On Diff #159961)

Does this line add anything? If not, can we remove it?

test/tools/llvm-objcopy/binary-input-arm.test

1 ↗

(On Diff #159961)

Could you fold all of the regular tests into a single test file? There isn't much difference between them. Just run llvm-objcopy N times in a single file with different -B options, and use check prefixes to match the different sections (e.g. COMMON, ARM, AARCH64... etc).

test/tools/llvm-objcopy/binary-input-error.test

1 ↗

(On Diff #159961)

printf -> echo

test/tools/llvm-objcopy/binary-input-reconstitute.test

1 ↗

(On Diff #159961)

I'm not sure I understand what "reconstitute" means here. Perhaps rename the test? I assume the aim is for binary input and binary output? So maybe a better name is "binary-input-and-output.test".

10 ↗

(On Diff #159961)

Technically, this only shows that the contents include "abcd", not that they are explicitly and solely "abcd". So this would also match a file containing other junk.

A better way might be to just compare the output files against the original input files using diff. Also, a common technique in our tests is to copy the input file to a separate file before running llvm-objcopy and then diff the input against the copy to show that the input hasn't been modified.

test/tools/llvm-objcopy/binary-input.test

2 ↗

(On Diff #159961)

I'm not sure how this test is different from the others (apart from the symbols). Is i386:x86-64 just a synonym of x86-64? If so, I think this test can be folded into the others, like mentioned above.

tools/llvm-objcopy/Object.cpp

66 ↗

(On Diff #159961)

Could you do these little unrelated tidy-ups in a separate NFC commit, please. No need for a review. Essentially, I'd like this review to just be of the things required for your change.

621 ↗

(On Diff #159961)

AR -> Data. "AR" is meaningless.

It saddens me that different parts of LLVM can't agree on whether to use chars or uint8_t.

624 ↗

(On Diff #159961)

Rather than abbreviate this to something that is unclear, just call it "DataSection"

633 ↗

(On Diff #159961)

I think it's considered bad form to have explicit local variable Twines that aren't just function parameters, if I remember, based on comments in a previous review (I might be mistaken though, so am happy to be proven wrong).

Please also name it something more descriptive like "Prefix".

642 ↗

(On Diff #159961)

Should the end symbol have a size? That seems weird that it does. I might expect the start symbol to, but not the end symbol. What does GNU objcopy do?

1249 ↗

(On Diff #159961)

Could you rename this initEhdrSegment, since it doesn't actually initialise the ELF header itself.

1261 ↗

(On Diff #159961)

I'm not convinced that the ELF header segment should be initialised inside something called "assignOffsets", since it's doing a lot more than assigning it an offset (which actually is always 0, so could be initialised at the same time as the rest of the header). It should probably be called outside this function.

tools/llvm-objcopy/Object.h

402 ↗

(On Diff #159961)

Side note: I think a recent change for --prefix-symbols makes this part of the diff identical. I recommend rebasing when you next update the diff.

457 ↗

(On Diff #159961)

Don't abbreviate variable names: Sz -> Size.

tools/llvm-objcopy/llvm-objcopy.cpp

133 ↗

(On Diff #159961)

It looks to me like we are grouping things in CopyConfig by type, so this should probably be moved to before (or after) the StringRef block.

302 ↗

(On Diff #159961)

I feel like this might read better if everything is constructed inline:

static const StringMap<MachineInfo> ArchMap{
    // Name,     EM value,   64bit, LittleEndian
    {"aarch64", {EM_AARCH64, true,  true}},
    {"arm",     {EM_ARM,     false, true}},
    /* More entries here */
};

I'd order the entries either alphabetically by name or numerically by their EM value. You could also put "headers" as shown to avoid needing to duplicate the 64/Endianness comment in each part.

[rupprecht]

• Fold arch-specific file header tests into a single templated test
• Replace printf w/ echo -n
• Other code review comments

[rupprecht]

test/tools/llvm-objcopy/binary-input-aarch64.test

1 ↗

(On Diff #159961)

Done -- I think printf was chosen to avoid the \n, but that can be done with "echo -n".

Looks like lit does support it, as it's used in test/tools/llvm-objcopy/add-gnu-debuglink.test, as well as some non-objcopy tests. I'll resist the temptation to fix those in this patch :)

3 ↗

(On Diff #159961)

Actually, the point of these arch-specific tests is just to check the file header. I removed -sections and -symbols from this test; those are checked by binary-input.test, which shouldn't depend much on the specific arch.

test/tools/llvm-objcopy/binary-input-reconstitute.test

1 ↗

(On Diff #159961)

Yes, essentially it's an integration test that some payload doesn't change when it's converted back and forth (i.e. payload -> object file w/ payload shoved into .data -> payload). I'm not sure why I thought "reconstitute" would be a good word for that, so I'll take your name suggestion.

test/tools/llvm-objcopy/binary-input.test

2 ↗

(On Diff #159961)

This test is focused on the generic stuff, namely section/symbols. I dropped the arch-specific stuff since that's covered elsewhere.

tools/llvm-objcopy/Object.cpp

633 ↗

(On Diff #159961)

I originally had string, but changed to Twine based on Jake's suggestion here: https://reviews.llvm.org/D50343?id=159343#inline-442972

I'm trying to understand why it would be a bad idea to write this. Looking at http://llvm.org/docs/ProgrammersManual.html#dss-twine, the discouraged pattern is:

void foo(const Twine &T);
...
StringRef X = ...
unsigned i = ...
const Twine &Tmp = X + "." + Twine(i);
foo(Tmp);

Which is bad because Tmp is a ref, whereas the Twine here is a regular stack variable that persists past all the calls to addSymbol (which calls .str() on the input twine to save/own the symbol).

I renamed to "Prefix", but kept this as a Twine here, since I think this is safe. Otherwise, I think I'd have to do:

SymTab->addSymbol(Twine("_binary_") + SanitizedFilename + "_start", ...);
SymTab->addSymbol(Twine("_binary_") + SanitizedFilename + "_end", ...);
SymTab->addSymbol(Twine("_binary_") + SanitizedFilename + "_size", ...);

642 ↗

(On Diff #159961)

Nope, this is Value, not Size, although it's confusing, especially because addSymbol has so many parameters. I commented the param names here to make this clear.
The sizes/values here match gnu objcopy.

tools/llvm-objcopy/Object.h

457 ↗

(On Diff #159961)

Done, although this is existing code -- it's only showing up as a diff because clang-format put it on a new line after I changed StringRef->Twine...

tools/llvm-objcopy/llvm-objcopy.cpp

133 ↗

(On Diff #159961)

Done -- I added some comments to try to logically explain how this config is laid out, but I'm not very familiar with all of them... I'm open to suggestions here.

[rupprecht]

Rebase

[jhenderson]

test/tools/llvm-objcopy/binary-input-and-output.test

1 ↗

(On Diff #160163)

Is the no new line important to this test? I think it isn't any more.

tools/llvm-objcopy/Object.cpp

642 ↗

(On Diff #159961)

Right, I get it now. Thanks for the comments.

tools/llvm-objcopy/Object.h

457 ↗

(On Diff #159961)

Ah, yes of course. But the renaming is good anyway.

703 ↗

(On Diff #160163)

If this is a clang-format added difference, please do it in another patch.

tools/llvm-objcopy/llvm-objcopy.cpp

133 ↗

(On Diff #159961)

I'd like @jakehehrlich to comment on these comments, if that's okay, as he may have a specific desire as to how this class is laid out.

[rupprecht]

• echo -n -> echo where it doesn't matter

[jakehehrlich]

tools/llvm-objcopy/llvm-objcopy.cpp

133 ↗

(On Diff #159961)

To date I have had no reason or rhyme to how I've added these...in fact I'm not sure if I have even added the majority at this point. A pattern of grouping by type does seem to have formed organically however. Let's stick with it.

Long term ideals on how this should be laid out:

1. The names should have a consistent method for naming them derived from the option name where possible. I don't think I've done a very good job of this and it isn't clear how to best solve this issue.
2. Grouping first by type and then by alphabetization is probably a good idea. I'm pretty trash at finding things in alphabetized lists but I always know the basic type of thing I'm looking for. AddSection is the only thing I know of which currently violates this. (we should make it work the way Paul made SectionsToRename work).

134–135 ↗

(On Diff #160409)

Do we use these anywhere? I think I added them thinking about how I was going to use them to do exactly what this change does but embedding MachineInfo seems much nicer and convey's the same information. Maybe we should just do that?

[rupprecht]

tools/llvm-objcopy/llvm-objcopy.cpp

134–135 ↗

(On Diff #160409)

We do. Essentially, we use this to decide whether we're going to need a BinaryReader vs ELFReader for InputFormat, and, separately, whether we're going to need a BinaryWriter vs ELFWriter for OutputFormat. BinaryArch is extra information that only applies when using BinaryReader.

[rupprecht]

• Rebase/fix other method names to be lower cased.

[jhenderson]

No null symbol is a blocker, so I've marked this as requesting changes. Sorry!

test/tools/llvm-objcopy/binary-input-and-output.test

5 ↗

(On Diff #160461)

I'm not sure if this is important or not in this case, but the usual pattern in llvm-objcopy tests is to copy the input file to a backup to verify that it hasn't been modified before doing the diff.

test/tools/llvm-objcopy/binary-input-arch.test

1 ↗

(On Diff #160461)

I don't think -n is important here? Indeed, you could do away with this file entirely and just feed in %s into llvm-objcopy, if you wanted, although this is probably less confusing.

test/tools/llvm-objcopy/binary-input-error.test

1 ↗

(On Diff #160461)

The -n isn't important here either.

test/tools/llvm-objcopy/binary-input.test

73 ↗

(On Diff #160461)

This is an illegal symbol table: it has no null symbol.

tools/llvm-objcopy/Object.cpp

613 ↗

(On Diff #160461)

Doesn't this assignment rely on knowing that the symbol table is added immediately after the string table? That seems like poor design to me. Better would be to pass in the index or string table section.

I might well be forgetting how llvm-objcopy is designed in this area, but don't we need to explicitly add a null symbol as the first symbol in the symbol table?

639–640 ↗

(On Diff #160461)

Is there any point in adding this local section symbol? It can't be referenced, so I think it's superfluous.

[rupprecht]

• Fix tests, add null symbol to symtab, make binary objection creation less fragile, and automatically assign section indices

tools/llvm-objcopy/Object.cpp

613 ↗

(On Diff #160461)

Yes, this is very fragile, I think I understand the issue I was having before a little better.

The Link needs to be set to the StrTab Index, as you mention. The Object::addSection() helper was not assigning any index, so this would implicitly be zero (which is SHN_UNDEF), and that was throwing errors when initializing it. Using size(Obj->sections()) - 1 was more of a reverse-engineered way of getting things working.

Changing Object::addSection() to automatically assign the index lets us use the index from StrTab directly. I think this might allow us to stop assigning manually indices elsewhere in this file, but I'll save that for another change.

Also -- fixed the symbol table to include a null symbol.

639–640 ↗

(On Diff #160461)

GNU objcopy adds it, but it doesn't seem to be necessary. I'll add it back if it turns out to be needed.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

613 ↗

(On Diff #160461)

Yeah that probably should have functioned that way the whole time. Thanks for fixing that!

[jhenderson]

LGTM, but there is one thing still outstanding, I think, which @jakehehrlich mentioned in a comment regarding the CopyConfig options:

A pattern of grouping by type does seem to have formed organically however. Let's stick with it.

I don't have a strong preference either way, but you should get @jakehehrlich to confirm he is happy before committing this change.

[rupprecht]

In D50343#1200318, @jhenderson wrote:

LGTM, but there is one thing still outstanding, I think, which @jakehehrlich mentioned in a comment regarding the CopyConfig options:

A pattern of grouping by type does seem to have formed organically however. Let's stick with it.

I don't have a strong preference either way, but you should get @jakehehrlich to confirm he is happy before committing this change.

Ok, I split out each section by type + alphabetized (as suggested after that comment). I also don't have any strong preference though; I'm happy to apply whatever organization/comments/etc. others want here.

There was also a suggestion in that comment to rename config names to more closely match flag names which is also a good idea, but I'd rather submit that as an NFC after this to avoid putting myself in merge conflict hell :)

[rupprecht]

• Reorganize CopyConfig sections

[rupprecht]

rebase/fix stale filename in a test case

llvm/trunk/test/tools/llvm-objcopy/binary-input-and-output.test

+# RUN: echo abcd > %t.txt
+
+# Preserve input to verify it is not modified
+# RUN: cp %t.txt %t-copy.txt
+
+# -I binary -O binary preserves payload through in-memory representation
+# RUN: llvm-objcopy -I binary -B i386:x86-64 -O binary %t.txt %t.2.txt
+# RUN: cmp %t.txt %t.2.txt
+# RUN: cmp %t.txt %t-copy.txt
+
+# -I binary -O binary preserves payload through an intermediate object file
+# RUN: llvm-objcopy -I binary -B i386:x86-64 %t.txt %t.o
+# RUN: llvm-objcopy -O binary %t.o %t.3.txt
+# RUN: cmp %t.txt %t.3.txt
+# RUN: cmp %t.txt %t-copy.txt

llvm/trunk/test/tools/llvm-objcopy/binary-input-arch.test

+# RUN: echo abcd > %t.txt
+
+# RUN: llvm-objcopy -I binary -B aarch64 %t.txt %t.aarch64.o
+# RUN: llvm-readobj -file-headers %t.aarch64.o | FileCheck %s --check-prefixes=CHECK,AARCH64,64
+
+# RUN: llvm-objcopy -I binary -B arm %t.txt %t.arm.o
+# RUN: llvm-readobj -file-headers %t.arm.o | FileCheck %s --check-prefixes=CHECK,ARM,32
+
+# RUN: llvm-objcopy -I binary -B i386 %t.txt %t.i386.o
+# RUN: llvm-readobj -file-headers %t.i386.o | FileCheck %s --check-prefixes=CHECK,I386,32
+
+# RUN: llvm-objcopy -I binary -B i386:x86-64 %t.txt %t.i386:x86-64.o
+# RUN: llvm-readobj -file-headers %t.i386:x86-64.o | FileCheck %s --check-prefixes=CHECK,X86-64,64
+
+# RUN: llvm-objcopy -I binary -B powerpc:common64 %t.txt %t.powerpc:common64.o
+# RUN: llvm-readobj -file-headers %t.powerpc:common64.o | FileCheck %s --check-prefixes=CHECK,PPC,64
+
+# RUN: llvm-objcopy -I binary -B sparc %t.txt %t.sparc.o
+# RUN: llvm-readobj -file-headers %t.sparc.o | FileCheck %s --check-prefixes=CHECK,SPARC,32
+
+# RUN: llvm-objcopy -I binary -B x86-64 %t.txt %t.x86-64.o
+# RUN: llvm-readobj -file-headers %t.x86-64.o | FileCheck %s --check-prefixes=CHECK,X86-64,64
+
+# CHECK: Format:
+# AARCH64-SAME: ELF64-aarch64-little
+# ARM-SAME:     ELF32-arm-little
+# I386-SAME:    ELF32-i386
+# PPC-SAME:     ELF64-ppc64
+# SPARC-SAME:   ELF32-sparc
+# X86-64-SAME:  ELF64-x86-64
+
+# AARCH64-NEXT: Arch: aarch64
+# ARM-NEXT:     Arch: arm
+# I386-NEXT:    Arch: i386
+# PPC-NEXT:     Arch: powerpc64le
+# SPARC-NEXT:   Arch: sparcel
+# X86-64-NEXT:  Arch: x86_64
+
+# 32-NEXT:      AddressSize: 32bit
+# 64-NEXT:      AddressSize: 64bit
+
+# CHECK:        ElfHeader {
+# CHECK-NEXT:     Ident {
+# CHECK-NEXT:       Magic: (7F 45 4C 46)
+# 32-NEXT:          Class: 32-bit (0x1)
+# 64-NEXT:          Class: 64-bit (0x2)
+# CHECK-NEXT:       DataEncoding: LittleEndian (0x1)
+# CHECK-NEXT:       FileVersion: 1
+# CHECK-NEXT:       OS/ABI: SystemV (0x0)
+# CHECK-NEXT:       ABIVersion: 0
+# CHECK-NEXT:       Unused: (00 00 00 00 00 00 00)
+# CHECK-NEXT:     }
+# CHECK-NEXT:     Type: Relocatable (0x1)
+# AARCH64-NEXT:   Machine: EM_AARCH64 (0xB7)
+# ARM-NEXT:       Machine: EM_ARM (0x28)
+# I386-NEXT:      Machine: EM_386 (0x3)
+# PPC-NEXT:       Machine: EM_PPC64 (0x15)
+# SPARC-NEXT:     Machine: EM_SPARC (0x2)
+# X86-64-NEXT:    Machine: EM_X86_64 (0x3E)
+# CHECK-NEXT:     Version: 1
+# CHECK-NEXT:     Entry: 0x0
+# CHECK-NEXT:     ProgramHeaderOffset:
+# CHECK-NEXT:     SectionHeaderOffset:
+# CHECK-NEXT:     Flags [ (0x0)
+# CHECK-NEXT:     ]
+# 32-NEXT:        HeaderSize: 52
+# 64-NEXT:        HeaderSize: 64
+# 32-NEXT:        ProgramHeaderEntrySize: 32
+# 64-NEXT:        ProgramHeaderEntrySize: 56
+# CHECK-NEXT:     ProgramHeaderCount: 0
+# 32-NEXT:        SectionHeaderEntrySize: 40
+# 64-NEXT:        SectionHeaderEntrySize: 64
+# CHECK-NEXT:     SectionHeaderCount: 4
+# CHECK-NEXT:     StringTableSectionIndex:
+# CHECK-NEXT:   }

llvm/trunk/test/tools/llvm-objcopy/binary-input-error.test

+# RUN: echo abcd > %t.txt
+
+# RUN: not llvm-objcopy -I binary %t.txt %t.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=MISSING-BINARY-ARCH
+
+# RUN: not llvm-objcopy -I binary -B xyz %t.txt %t.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=BAD-BINARY-ARCH
+
+# MISSING-BINARY-ARCH: Specified binary input without specifiying an architecture.
+# BAD-BINARY-ARCH: Invalid architecture: 'xyz'.

llvm/trunk/test/tools/llvm-objcopy/binary-input.test

+# RUN: echo -n abcd > %t.x-txt
+# Preserve input to verify it is not modified
+# RUN: cp %t.x-txt %t-copy.txt
+# RUN: llvm-objcopy -I binary -B i386:x86-64 %t.x-txt %t.o
+# RUN: llvm-readobj -sections -symbols %t.o | FileCheck %s
+# RUN: cmp %t.x-txt %t-copy.txt
+
+# CHECK:      Sections [
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 0
+# CHECK-NEXT:     Name:  (0)
+# CHECK-NEXT:     Type: SHT_NULL (0x0)
+# CHECK-NEXT:     Flags [ (0x0)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: 0x0
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Link: 0
+# CHECK-NEXT:     Info: 0
+# CHECK-NEXT:     AddressAlignment: 0
+# CHECK-NEXT:     EntrySize: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 1
+# CHECK-NEXT:     Name: .strtab
+# CHECK-NEXT:     Type: SHT_STRTAB (0x3)
+# CHECK-NEXT:     Flags [ (0x0)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: 0x0
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Link: 0
+# CHECK-NEXT:     Info: 0
+# CHECK-NEXT:     AddressAlignment: 1
+# CHECK-NEXT:     EntrySize: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 2
+# CHECK-NEXT:     Name: .symtab
+# CHECK-NEXT:     Type: SHT_SYMTAB (0x2)
+# CHECK-NEXT:     Flags [ (0x0)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: 0x0
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Link: 1
+# CHECK-NEXT:     Info: 1
+# CHECK-NEXT:     AddressAlignment: 1
+# CHECK-NEXT:     EntrySize: 24
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 3
+# CHECK-NEXT:     Name: .data
+# CHECK-NEXT:     Type: SHT_PROGBITS (0x1)
+# CHECK-NEXT:     Flags [ (0x3)
+# CHECK-NEXT:       SHF_ALLOC (0x2)
+# CHECK-NEXT:       SHF_WRITE (0x1)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: 0x0
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     Size: 4
+# CHECK-NEXT:     Link: 0
+# CHECK-NEXT:     Info: 0
+# CHECK-NEXT:     AddressAlignment: 1
+# CHECK-NEXT:     EntrySize: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+# Note: the symbol names are derived from the full path (with non-alnum values
+# replaced with "_"), e.g. "/tmp/a-b.c" should yield
+# _binary__tmp_a_b_c_{start,end,size}.
+# Just check for _binary_{{[_a-zA-Z0-9]*}}_x_txt_{start,end,size} to avoid
+# making assumptions about how this test is run.
+
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name:
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Local (0x0)
+# CHECK-NEXT:     Type: None (0x0)
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: Undefined (0x0)
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: _binary_{{[_a-zA-Z0-9]*}}_x_txt_start
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Global (0x1)
+# CHECK-NEXT:     Type: None (0x0)
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: .data
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: _binary_{{[_a-zA-Z0-9]*}}_x_txt_end
+# CHECK-NEXT:     Value: 0x4
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Global (0x1)
+# CHECK-NEXT:     Type: None (0x0)
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: .data
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: _binary_{{[_a-zA-Z0-9]*}}_x_txt_size
+# CHECK-NEXT:     Value: 0x4
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Global (0x1)
+# CHECK-NEXT:     Type: None (0x0)
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: Absolute
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]

llvm/trunk/tools/llvm-objcopy/Object.h

public:
   SectionBase *getSection(uint32_t Index, Twine ErrMsg);
 
   template <class T>
   T *getSectionOfType(uint32_t Index, Twine IndexErrMsg, Twine TypeErrMsg);
 };
 
 enum ElfType { ELFT_ELF32LE, ELFT_ELF64LE, ELFT_ELF32BE, ELFT_ELF64BE };
 
+// This type keeps track of the machine info for various architectures. This
+// lets us map architecture names to ELF types and the e_machine value of the
+// ELF file.
+struct MachineInfo {
+  uint16_t EMachine;
+  bool Is64Bit;
+  bool IsLittleEndian;
+};
+
 class SectionVisitor {
 public:
   virtual ~SectionVisitor();
 
   virtual void visit(const Section &Sec) = 0;
   virtual void visit(const OwnedDataSection &Sec) = 0;
   virtual void visit(const StringTableSection &Sec) = 0;
   virtual void visit(const SymbolTableSection &Sec) = 0;

 
 template <class ELFT> class ELFWriter : public Writer {
 private:
   using Elf_Addr = typename ELFT::Addr;
   using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Phdr = typename ELFT::Phdr;
   using Elf_Ehdr = typename ELFT::Ehdr;
 
+  void initEhdrSegment();
+
   void writeEhdr();
   void writePhdr(const Segment &Seg);
   void writeShdr(const SectionBase &Sec);
 
   void writePhdrs();
   void writeShdrs();
   void writeSectionData();
 

 protected:
   std::vector<std::unique_ptr<Symbol>> Symbols;
   StringTableSection *SymbolNames = nullptr;
   SectionIndexSection *SectionIndexTable = nullptr;
 
   using SymPtr = std::unique_ptr<Symbol>;
 
 public:
-  void addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
-                 SectionBase *DefinedIn, uint64_t Value, uint8_t Visibility,
-                 uint16_t Shndx, uint64_t Sz);
+  SymbolTableSection() { Type = ELF::SHT_SYMTAB; }
+
+  void addSymbol(Twine Name, uint8_t Bind, uint8_t Type, SectionBase *DefinedIn,
+                 uint64_t Value, uint8_t Visibility, uint16_t Shndx,
+                 uint64_t Size);
   void prepareForLayout();
   // An 'empty' symbol table still contains a null symbol.
   bool empty() const { return Symbols.size() == 1; }
   void setShndxTable(SectionIndexSection *ShndxTable) {
     SectionIndexTable = ShndxTable;
   }
   const SectionIndexSection *getShndxTable() const { return SectionIndexTable; }
   const SectionBase *getStrTab() const { return SymbolNames; }
public:
   virtual std::unique_ptr<Object> create() const = 0;
 };
 
 using object::Binary;
 using object::ELFFile;
 using object::ELFObjectFile;
 using object::OwningBinary;
 
+template <class ELFT> class BinaryELFBuilder {
+  using Elf_Sym = typename ELFT::Sym;
+
+  uint16_t EMachine;
+  MemoryBuffer *MemBuf;
+  std::unique_ptr<Object> Obj;
+
+  void initFileHeader();
+  void initHeaderSegment();
+  StringTableSection *addStrTab();
+  SymbolTableSection *addSymTab(StringTableSection *StrTab);
+  void addData(SymbolTableSection *SymTab);
+  void initSections();
+
+public:
+  BinaryELFBuilder(uint16_t EM, MemoryBuffer *MB)
+      : EMachine(EM), MemBuf(MB), Obj(llvm::make_unique<Object>()) {}
+
+  std::unique_ptr<Object> build();
+};
+
 template <class ELFT> class ELFBuilder {
 private:
   using Elf_Addr = typename ELFT::Addr;
   using Elf_Shdr = typename ELFT::Shdr;
-  using Elf_Ehdr = typename ELFT::Ehdr;
   using Elf_Word = typename ELFT::Word;
 
   const ELFFile<ELFT> &ElfFile;
   Object &Obj;
 
   void setParentSegment(Segment &Child);
   void readProgramHeaders();
   void initGroupSection(GroupSection *GroupSec);
   void initSymbolTable(SymbolTableSection *SymTab);
   void readSectionHeaders();
   SectionBase &makeSection(const Elf_Shdr &Shdr);
 
 public:
   ELFBuilder(const ELFObjectFile<ELFT> &ElfObj, Object &Obj)
       : ElfFile(*ElfObj.getELFFile()), Obj(Obj) {}
 
   void build();
 };
 
+class BinaryReader : public Reader {
+  const MachineInfo &MInfo;
+  MemoryBuffer *MemBuf;
+
+public:
+  BinaryReader(const MachineInfo &MI, MemoryBuffer *MB)
+      : MInfo(MI), MemBuf(MB) {}
+  std::unique_ptr<Object> create() const override;
+};
+
 class ELFReader : public Reader {
   Binary *Bin;
 
 public:
-  ElfType getElfType() const;
   std::unique_ptr<Object> create() const override;
   explicit ELFReader(Binary *B) : Bin(B) {}
 };
 
 class Object {
 private:
   using SecPtr = std::unique_ptr<SectionBase>;
   using SegPtr = std::unique_ptr<Segment>;
public:
   // not present in any segment. This could be a problem during file layout,
   // because other segments may get assigned an offset where either of the
   // two should reside, which will effectively corrupt the resulting binary.
   // Other than that we use these segments to track program header offsets
   // when they may not follow the ELF header.
   Segment ElfHdrSegment;
   Segment ProgramHdrSegment;
 
-  uint8_t Ident[16];
   uint64_t Entry;
   uint64_t SHOffset;
   uint32_t Type;
   uint32_t Machine;
   uint32_t Version;
   uint32_t Flags;
 
   StringTableSection *SectionNames = nullptr;
public:
   ConstRange<Segment> segments() const { return make_pointee_range(Segments); }
 
   void removeSections(std::function<bool(const SectionBase &)> ToRemove);
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   template <class T, class... Ts> T &addSection(Ts &&... Args) {
     auto Sec = llvm::make_unique<T>(std::forward<Ts>(Args)...);
     auto Ptr = Sec.get();
     Sections.emplace_back(std::move(Sec));
+    Ptr->Index = Sections.size();
     return *Ptr;
   }
   Segment &addSegment(ArrayRef<uint8_t> Data) {
     Segments.emplace_back(llvm::make_unique<Segment>(Data));
     return *Segments.back();
   }
 };
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_OBJECT_H

llvm/trunk/tools/llvm-objcopy/Object.cpp

 }
 
 void SymbolTableSection::assignIndices() {
   uint32_t Index = 0;
   for (auto &Sym : Symbols)
     Sym->Index = Index++;
 }
 
-void SymbolTableSection::addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
+void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
                                    SectionBase *DefinedIn, uint64_t Value,
                                    uint8_t Visibility, uint16_t Shndx,
-                                   uint64_t Sz) {
+                                   uint64_t Size) {
   Symbol Sym;
-  Sym.Name = Name;
+  Sym.Name = Name.str();
   Sym.Binding = Bind;
   Sym.Type = Type;
   Sym.DefinedIn = DefinedIn;
   if (DefinedIn != nullptr)
     DefinedIn->HasSymbol = true;
   if (DefinedIn == nullptr) {
     if (Shndx >= SHN_LORESERVE)
       Sym.ShndxType = static_cast<SymbolShndxType>(Shndx);
     else
       Sym.ShndxType = SYMBOL_SIMPLE_INDEX;
   }
   Sym.Value = Value;
   Sym.Visibility = Visibility;
-  Sym.Size = Sz;
+  Sym.Size = Size;
   Sym.Index = Symbols.size();
   Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
   Size += this->EntrySize;
 }
 
 void SymbolTableSection::removeSectionReferences(const SectionBase *Sec) {
   if (SectionIndexTable == Sec)
     SectionIndexTable = nullptr;

 static bool compareSegmentsByPAddr(const Segment *A, const Segment *B) {
   if (A->PAddr < B->PAddr)
     return true;
   if (A->PAddr > B->PAddr)
     return false;
   return A->Index < B->Index;
 }
 
+template <class ELFT> void BinaryELFBuilder<ELFT>::initFileHeader() {
+  Obj->Flags = 0x0;
+  Obj->Type = ET_REL;
+  Obj->Entry = 0x0;
+  Obj->Machine = EMachine;
+  Obj->Version = 1;
+}
+
+template <class ELFT> void BinaryELFBuilder<ELFT>::initHeaderSegment() {
+  Obj->ElfHdrSegment.Index = 0;
+}
+
+template <class ELFT> StringTableSection *BinaryELFBuilder<ELFT>::addStrTab() {
+  auto &StrTab = Obj->addSection<StringTableSection>();
+  StrTab.Name = ".strtab";
+
+  Obj->SectionNames = &StrTab;
+  return &StrTab;
+}
+
+template <class ELFT>
+SymbolTableSection *
+BinaryELFBuilder<ELFT>::addSymTab(StringTableSection *StrTab) {
+  auto &SymTab = Obj->addSection<SymbolTableSection>();
+
+  SymTab.Name = ".symtab";
+  SymTab.Link = StrTab->Index;
+  // TODO: Factor out dependence on ElfType here.
+  SymTab.EntrySize = sizeof(Elf_Sym);
+
+  // The symbol table always needs a null symbol
+  SymTab.addSymbol("", 0, 0, nullptr, 0, 0, 0, 0);
+
+  Obj->SymbolTable = &SymTab;
+  return &SymTab;
+}
+
+template <class ELFT>
+void BinaryELFBuilder<ELFT>::addData(SymbolTableSection *SymTab) {
+  auto Data = ArrayRef<uint8_t>(
+      reinterpret_cast<const uint8_t *>(MemBuf->getBufferStart()),
+      MemBuf->getBufferSize());
+  auto &DataSection = Obj->addSection<Section>(Data);
+  DataSection.Name = ".data";
+  DataSection.Type = ELF::SHT_PROGBITS;
+  DataSection.Size = Data.size();
+  DataSection.Flags = ELF::SHF_ALLOC | ELF::SHF_WRITE;
+
+  std::string SanitizedFilename = MemBuf->getBufferIdentifier().str();
+  std::replace_if(std::begin(SanitizedFilename), std::end(SanitizedFilename),
+                  [](char c) { return !isalnum(c); }, '_');
+  Twine Prefix = Twine("_binary_") + SanitizedFilename;
+
+  SymTab->addSymbol(Prefix + "_start", STB_GLOBAL, STT_NOTYPE, &DataSection,
+                    /*Value=*/0, STV_DEFAULT, 0, 0);
+  SymTab->addSymbol(Prefix + "_end", STB_GLOBAL, STT_NOTYPE, &DataSection,
+                    /*Value=*/DataSection.Size, STV_DEFAULT, 0, 0);
+  SymTab->addSymbol(Prefix + "_size", STB_GLOBAL, STT_NOTYPE, nullptr,
+                    /*Value=*/DataSection.Size, STV_DEFAULT, SHN_ABS, 0);
+}
+
+template <class ELFT> void BinaryELFBuilder<ELFT>::initSections() {
+  for (auto &Section : Obj->sections()) {
+    Section.initialize(Obj->sections());
+  }
+}
+
+template <class ELFT> std::unique_ptr<Object> BinaryELFBuilder<ELFT>::build() {
+  initFileHeader();
+  initHeaderSegment();
+  StringTableSection *StrTab = addStrTab();
+  SymbolTableSection *SymTab = addSymTab(StrTab);
+  initSections();
+  addData(SymTab);
+
+  return std::move(Obj);
+}
+
 template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) {
   for (auto &Parent : Obj.segments()) {
     // Every segment will overlap with itself but we don't want a segment to
     // be it's own parent so we avoid that situation.
     if (&Child != &Parent && segmentOverlapsSegment(Child, Parent)) {
       // We want a canonical "most parental" segment but this requires
       // inspecting the ParentSegment.
       if (compareSegmentsByOffset(&Parent, &Child))
for (auto &Section : Obj.sections()) {
             Section.ParentSegment->Offset > Seg.Offset) {
           Section.ParentSegment = &Seg;
         }
       }
     }
   }
 
   auto &ElfHdr = Obj.ElfHdrSegment;
-  // Creating multiple PT_PHDR segments technically is not valid, but PT_LOAD
-  // segments must not overlap, and other types fit even less.
-  ElfHdr.Type = PT_PHDR;
-  ElfHdr.Flags = 0;
-  ElfHdr.OriginalOffset = ElfHdr.Offset = 0;
-  ElfHdr.VAddr = 0;
-  ElfHdr.PAddr = 0;
-  ElfHdr.FileSize = ElfHdr.MemSize = sizeof(Elf_Ehdr);
-  ElfHdr.Align = 0;
   ElfHdr.Index = Index++;
 
   const auto &Ehdr = *ElfFile.getHeader();
   auto &PrHdr = Obj.ProgramHdrSegment;
   PrHdr.Type = PT_PHDR;
   PrHdr.Flags = 0;
   // The spec requires us to have p_vaddr % p_align == p_offset % p_align.
   // Whereas this works automatically for ElfHdr, here OriginalOffset is
if (auto RelSec = dyn_cast<RelocationSection>(&Section)) {
       initGroupSection(GroupSec);
     }
   }
 }
 
 template <class ELFT> void ELFBuilder<ELFT>::build() {
   const auto &Ehdr = *ElfFile.getHeader();
 
-  std::copy(Ehdr.e_ident, Ehdr.e_ident + 16, Obj.Ident);
   Obj.Type = Ehdr.e_type;
   Obj.Machine = Ehdr.e_machine;
   Obj.Version = Ehdr.e_version;
   Obj.Entry = Ehdr.e_entry;
   Obj.Flags = Ehdr.e_flags;
 
   readSectionHeaders();
   readProgramHeaders();

 template <class R> size_t size(R &&Range) {
   return static_cast<size_t>(std::end(Range) - std::begin(Range));
 }
 
 Writer::~Writer() {}
 
 Reader::~Reader() {}
 
-ElfType ELFReader::getElfType() const {
-  if (isa<ELFObjectFile<ELF32LE>>(Bin))
-    return ELFT_ELF32LE;
-  if (isa<ELFObjectFile<ELF64LE>>(Bin))
-    return ELFT_ELF64LE;
-  if (isa<ELFObjectFile<ELF32BE>>(Bin))
-    return ELFT_ELF32BE;
-  if (isa<ELFObjectFile<ELF64BE>>(Bin))
-    return ELFT_ELF64BE;
-  llvm_unreachable("Invalid ELFType");
+std::unique_ptr<Object> BinaryReader::create() const {
+  if (MInfo.Is64Bit)
+    return MInfo.IsLittleEndian
+               ? BinaryELFBuilder<ELF64LE>(MInfo.EMachine, MemBuf).build()
+               : BinaryELFBuilder<ELF64BE>(MInfo.EMachine, MemBuf).build();
+  else
+    return MInfo.IsLittleEndian
+               ? BinaryELFBuilder<ELF32LE>(MInfo.EMachine, MemBuf).build()
+               : BinaryELFBuilder<ELF32BE>(MInfo.EMachine, MemBuf).build();
 }
 
 std::unique_ptr<Object> ELFReader::create() const {
   auto Obj = llvm::make_unique<Object>();
   if (auto *o = dyn_cast<ELFObjectFile<ELF32LE>>(Bin)) {
     ELFBuilder<ELF32LE> Builder(*o, *Obj);
     Builder.build();
     return Obj;
if (auto *o = dyn_cast<ELFObjectFile<ELF32LE>>(Bin)) {
     return Obj;
   }
   error("Invalid file type");
 }
 
 template <class ELFT> void ELFWriter<ELFT>::writeEhdr() {
   uint8_t *B = Buf.getBufferStart();
   Elf_Ehdr &Ehdr = *reinterpret_cast<Elf_Ehdr *>(B);
-  std::copy(Obj.Ident, Obj.Ident + 16, Ehdr.e_ident);
+  std::fill(Ehdr.e_ident, Ehdr.e_ident + 16, 0);
+  Ehdr.e_ident[EI_MAG0] = 0x7f;
+  Ehdr.e_ident[EI_MAG1] = 'E';
+  Ehdr.e_ident[EI_MAG2] = 'L';
+  Ehdr.e_ident[EI_MAG3] = 'F';
+  Ehdr.e_ident[EI_CLASS] = ELFT::Is64Bits ? ELFCLASS64 : ELFCLASS32;
+  Ehdr.e_ident[EI_DATA] =
+      ELFT::TargetEndianness == support::big ? ELFDATA2MSB : ELFDATA2LSB;
+  Ehdr.e_ident[EI_VERSION] = EV_CURRENT;
+  Ehdr.e_ident[EI_OSABI] = ELFOSABI_NONE;
+  Ehdr.e_ident[EI_ABIVERSION] = 0;
+
   Ehdr.e_type = Obj.Type;
   Ehdr.e_machine = Obj.Machine;
   Ehdr.e_version = Obj.Version;
   Ehdr.e_entry = Obj.Entry;
+  // TODO: Only set phoff when a program header exists, to avoid tools
+  // thinking this is corrupt data.
   Ehdr.e_phoff = Obj.ProgramHdrSegment.Offset;
   Ehdr.e_flags = Obj.Flags;
   Ehdr.e_ehsize = sizeof(Elf_Ehdr);
   Ehdr.e_phentsize = sizeof(Elf_Phdr);
   Ehdr.e_phnum = size(Obj.segments());
   Ehdr.e_shentsize = sizeof(Elf_Shdr);
   if (WriteSectionHeaders) {
     Ehdr.e_shoff = Obj.SHOffset;
if (Section.ParentSegment != nullptr) {
       Section.Offset = Offset;
       if (Section.Type != SHT_NOBITS)
         Offset += Section.Size;
     }
   }
   return Offset;
 }
 
+template <class ELFT> void ELFWriter<ELFT>::initEhdrSegment() {
+  auto &ElfHdr = Obj.ElfHdrSegment;
+  ElfHdr.Type = PT_PHDR;
+  ElfHdr.Flags = 0;
+  ElfHdr.OriginalOffset = ElfHdr.Offset = 0;
+  ElfHdr.VAddr = 0;
+  ElfHdr.PAddr = 0;
+  ElfHdr.FileSize = ElfHdr.MemSize = sizeof(Elf_Ehdr);
+  ElfHdr.Align = 0;
+}
+
 template <class ELFT> void ELFWriter<ELFT>::assignOffsets() {
   // We need a temporary list of segments that has a special order to it
   // so that we know that anytime ->ParentSegment is set that segment has
   // already had its offset properly set.
   std::vector<Segment *> OrderedSegments;
   for (auto &Segment : Obj.segments())
     OrderedSegments.push_back(&Segment);
   OrderedSegments.push_back(&Obj.ElfHdrSegment);
template <class ELFT> void ELFWriter<ELFT>::finalize() {
 
   // Make sure we add the names of all the sections. Importantly this must be
   // done after we decide to add or remove SectionIndexes.
   if (Obj.SectionNames != nullptr)
     for (const auto &Section : Obj.sections()) {
       Obj.SectionNames->addString(Section.Name);
     }
 
+  initEhdrSegment();
   // Before we can prepare for layout the indexes need to be finalized.
   uint64_t Index = 0;
   for (auto &Sec : Obj.sections())
     Sec.Index = Index++;
 
   // The symbol table does not update all other sections on update. For
   // instance, symbol names are not added as new symbols are added. This means
   // that some sections, like .strtab, don't yet have their final size.
void BinaryWriter::finalize() {
 
   Buf.allocate(TotalSize);
   SecWriter = llvm::make_unique<BinarySectionWriter>(Buf);
 }
 
 namespace llvm {
 namespace objcopy {
 
+template class BinaryELFBuilder<ELF64LE>;
+template class BinaryELFBuilder<ELF64BE>;
+template class BinaryELFBuilder<ELF32LE>;
+template class BinaryELFBuilder<ELF32BE>;
+
 template class ELFBuilder<ELF64LE>;
 template class ELFBuilder<ELF64BE>;
 template class ELFBuilder<ELF32LE>;
 template class ELFBuilder<ELF32BE>;
 
 template class ELFWriter<ELF64LE>;
 template class ELFWriter<ELF64BE>;
 template class ELFWriter<ELF32LE>;
 template class ELFWriter<ELF32BE>;
 } // end namespace objcopy
 } // end namespace llvm

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp

 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/Memory.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <functional>

 
 struct SectionRename {
   StringRef OriginalName;
   StringRef NewName;
   Optional<uint64_t> NewFlags;
 };
 
 struct CopyConfig {
-  StringRef OutputFilename;
+  // Main input/output options
   StringRef InputFilename;
-  StringRef OutputFormat;
   StringRef InputFormat;
-  StringRef BinaryArch;
+  StringRef OutputFilename;
+  StringRef OutputFormat;
 
-  StringRef SplitDWO;
+  // Only applicable for --input-format=Binary
+  MachineInfo BinaryArch;
+
+  // Advanced options
   StringRef AddGnuDebugLink;
+  StringRef SplitDWO;
   StringRef SymbolsPrefix;
-  std::vector<StringRef> ToRemove;
-  std::vector<StringRef> Keep;
-  std::vector<StringRef> OnlyKeep;
+
+  // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
-  std::vector<StringRef> SymbolsToLocalize;
+  std::vector<StringRef> Keep;
+  std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> SymbolsToGlobalize;
-  std::vector<StringRef> SymbolsToWeaken;
-  std::vector<StringRef> SymbolsToRemove;
   std::vector<StringRef> SymbolsToKeep;
+  std::vector<StringRef> SymbolsToLocalize;
+  std::vector<StringRef> SymbolsToRemove;
+  std::vector<StringRef> SymbolsToWeaken;
+  std::vector<StringRef> ToRemove;
+
+  // Map options
   StringMap<SectionRename> SectionsToRename;
   StringMap<StringRef> SymbolsToRename;
+
+  // Boolean options
+  bool DiscardAll = false;
+  bool ExtractDWO = false;
+  bool KeepFileSymbols = false;
+  bool LocalizeHidden = false;
+  bool OnlyKeepDebug = false;
+  bool PreserveDates = false;
   bool StripAll = false;
   bool StripAllGNU = false;
+  bool StripDWO = false;
   bool StripDebug = false;
-  bool StripSections = false;
   bool StripNonAlloc = false;
-  bool StripDWO = false;
+  bool StripSections = false;
   bool StripUnneeded = false;
-  bool ExtractDWO = false;
-  bool LocalizeHidden = false;
   bool Weaken = false;
-  bool DiscardAll = false;
-  bool OnlyKeepDebug = false;
-  bool KeepFileSymbols = false;
-  bool PreserveDates = false;
 };
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
 enum SectionFlag {
   SecNone = 0,
   SecAlloc = 1 << 0,
   SecLoad = 1 << 1,
static bool onlyKeepDWOPred(const Object &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
   if (&Sec == Obj.SectionNames)
     return false;
   // Short of keeping the string table we want to keep everything that is a DWO
   // section and remove everything else.
   return !isDWOSection(Sec);
 }
 
+static const StringMap<MachineInfo> ArchMap{
+    // Name, {EMachine, 64bit, LittleEndian}
+    {"aarch64", {EM_AARCH64, true, true}},
+    {"arm", {EM_ARM, false, true}},
+    {"i386", {EM_386, false, true}},
+    {"i386:x86-64", {EM_X86_64, true, true}},
+    {"powerpc:common64", {EM_PPC64, true, true}},
+    {"sparc", {EM_SPARC, false, true}},
+    {"x86-64", {EM_X86_64, true, true}},
+};
+
+static const MachineInfo &getMachineInfo(StringRef Arch) {
+  auto Iter = ArchMap.find(Arch);
+  if (Iter == std::end(ArchMap))
+    error("Invalid architecture: '" + Arch + "'");
+  return Iter->getValue();
+}
+
+static ElfType getOutputElfType(const Binary &Bin) {
+  // Infer output ELF type from the input ELF object
+  if (isa<ELFObjectFile<ELF32LE>>(Bin))
+    return ELFT_ELF32LE;
+  if (isa<ELFObjectFile<ELF64LE>>(Bin))
+    return ELFT_ELF64LE;
+  if (isa<ELFObjectFile<ELF32BE>>(Bin))
+    return ELFT_ELF32BE;
+  if (isa<ELFObjectFile<ELF64BE>>(Bin))
+    return ELFT_ELF64BE;
+  llvm_unreachable("Invalid ELFType");
+}
+
+static ElfType getOutputElfType(const MachineInfo &MI) {
+  // Infer output ELF type from the binary arch specified
+  if (MI.Is64Bit)
+    return MI.IsLittleEndian ? ELFT_ELF64LE : ELFT_ELF64BE;
+  else
+    return MI.IsLittleEndian ? ELFT_ELF32LE : ELFT_ELF32BE;
+}
+
 static std::unique_ptr<Writer> createWriter(const CopyConfig &Config,
                                             Object &Obj, Buffer &Buf,
                                             ElfType OutputElfType) {
   if (Config.OutputFormat == "binary") {
     return llvm::make_unique<BinaryWriter>(Obj, Buf);
   }
   // Depending on the initial ELFT and OutputFormat we need a different Writer.
   switch (OutputElfType) {
for (const auto &Flag : Config.DumpSection) {
         reportError(Config.InputFilename, std::move(E));
     }
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
 }
 
-static void executeElfObjcopyOnBinary(const CopyConfig &Config, Binary &Binary,
-                                      Buffer &Out) {
-  ELFReader Reader(&Binary);
+static void executeElfObjcopyOnBinary(const CopyConfig &Config, Reader &Reader,
+                                      Buffer &Out, ElfType OutputElfType) {
   std::unique_ptr<Object> Obj = Reader.create();
 
-  handleArgs(Config, *Obj, Reader, Reader.getElfType());
+  handleArgs(Config, *Obj, Reader, OutputElfType);
 
   std::unique_ptr<Writer> Writer =
-      createWriter(Config, *Obj, Out, Reader.getElfType());
+      createWriter(Config, *Obj, Out, OutputElfType);
   Writer->finalize();
   Writer->write();
 }
 
 // For regular archives this function simply calls llvm::writeArchive,
 // For thin archives it writes the archive file itself as well as its members.
 static Error deepWriteArchive(StringRef ArcName,
                               ArrayRef<NewArchiveMember> NewMembers,

 static void executeElfObjcopyOnArchive(const CopyConfig &Config,
                                        const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
     if (!ChildOrErr)
       reportError(Ar.getFileName(), ChildOrErr.takeError());
+    Binary *Bin = ChildOrErr->get();
+
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
       reportError(Ar.getFileName(), ChildNameOrErr.takeError());
 
     MemBuffer MB(ChildNameOrErr.get());
-    executeElfObjcopyOnBinary(Config, **ChildOrErr, MB);
+    ELFReader Reader(Bin);
+    executeElfObjcopyOnBinary(Config, Reader, MB, getOutputElfType(*Bin));
 
     Expected<NewArchiveMember> Member =
         NewArchiveMember::getOldMember(Child, true);
     if (!Member)
       reportError(Ar.getFileName(), Member.takeError());
     Member->Buf = MB.releaseMemoryBuffer();
     Member->MemberName = Member->Buf->getBufferIdentifier();
     NewArchiveMembers.push_back(std::move(*Member));

 }
 
 static void executeElfObjcopy(const CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.PreserveDates)
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
       reportError(Config.InputFilename, EC);
 
+  if (Config.InputFormat == "binary") {
+    auto BufOrErr = MemoryBuffer::getFile(Config.InputFilename);
+    if (!BufOrErr)
+      reportError(Config.InputFilename, BufOrErr.getError());
+
+    FileBuffer FB(Config.OutputFilename);
+    BinaryReader Reader(Config.BinaryArch, BufOrErr->get());
+    executeElfObjcopyOnBinary(Config, Reader, FB,
+                              getOutputElfType(Config.BinaryArch));
+  } else {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     if (!BinaryOrErr)
       reportError(Config.InputFilename, BinaryOrErr.takeError());
 
     if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
       executeElfObjcopyOnArchive(Config, *Ar);
     } else {
       FileBuffer FB(Config.OutputFilename);
-    executeElfObjcopyOnBinary(Config, *BinaryOrErr.get().getBinary(), FB);
+      Binary *Bin = BinaryOrErr.get().getBinary();
+      ELFReader Reader(Bin);
+      executeElfObjcopyOnBinary(Config, Reader, FB, getOutputElfType(*Bin));
+    }
   }
 
   if (Config.PreserveDates) {
     restoreDateOnFile(Config.OutputFilename, Stat);
     if (!Config.SplitDWO.empty())
       restoreDateOnFile(Config.SplitDWO, Stat);
   }
 }
static CopyConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   if (Positional.size() > 2)
     error("Too many positional arguments");
 
   CopyConfig Config;
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
   Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
   Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
-  Config.BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
+  if (Config.InputFormat == "binary") {
+    auto BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
+    if (BinaryArch.empty())
+      error("Specified binary input without specifiying an architecture");
+    Config.BinaryArch = getMachineInfo(BinaryArch);
+  }
 
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
   Config.AddGnuDebugLink = InputArgs.getLastArgValue(OBJCOPY_add_gnu_debuglink);
   Config.SymbolsPrefix = InputArgs.getLastArgValue(OBJCOPY_prefix_symbols);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
       error("Bad format for --redefine-sym");