This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Recognize the output file format other than ELF
AbandonedPublic

Authored by seiya on Jun 12 2019, 12:25 AM.

Details

Summary

Currently, llvm-objcopy doesn't recognize the output file format given by the -O option and implicitly calls
elf::executeObjcopyOnRawBinary if the input is binary.

This patches changes the type of Config.InputFormat and Config.OutputFormat to an enum so that
we can call executeObjcopyOnRawBinary function for other output file formats like MachO in the future.

In addition to this, this patch adds -O option into tests some tests because it seems that GNU objcopy simply
copies the input file if -O is not specified, that is, if you run objcopy -Ibinary foo.txt bar, the output file "bar" is
identical "foo.txt", not an ELF object file.

Furthermore, this patch adds "elf32-sparc" format to fix the "-B sparc" test in binary-input-arch.test.

Diff Detail

Event Timeline

seiya created this revision.Jun 12 2019, 12:25 AM
seiya edited the summary of this revision. (Show Details)Jun 12 2019, 12:30 AM

@seiya, it looks like what you've observed in the tests is https://bugs.llvm.org/show_bug.cgi?id=42171. Can you confirm, and if so update the bug accordingly?

I feel like there might be three different patches here, rather than one big one. The first one is the bug fix (with corresponding test changes), the second is the sparc format issue, and the third is the work your title suggests this patch should be doing (i.e. changing the types). Please could you split it up, as it will make it easier to review each change separately.

seiya added a comment.Jun 12 2019, 4:14 AM

@seiya, it looks like what you've observed in the tests is https://bugs.llvm.org/show_bug.cgi?id=42171. Can you confirm, and if so update the bug accordingly?

I feel like there might be three different patches here, rather than one big one. The first one is the bug fix (with corresponding test changes), the second is the sparc format issue, and the third is the work your title suggests this patch should be doing (i.e. changing the types). Please could you split it up, as it will make it easier to review each change separately.

Thank you. I missed that. That bug is what I want to fix this patch. I'll split this into separate patches.

For what it's worth, the usual thing to do is to leave the "core" of these sort of changes in the original patch, and create one or more new patches for the other part(s).