Page MenuHomePhabricator

[llvm-objcopy][NFC] Refactor output target parsing
ClosedPublic

Authored by seiya on Jun 12 2019, 5:42 PM.

Details

Summary

Use an enum instead of string to hold the output file format in Config.InputFormat and Config.OutputFormat. It's essential to support other output file formats other than ELF.

Diff Detail

Repository
rL LLVM

Event Timeline

seiya created this revision.Jun 12 2019, 5:42 PM
compnerd added inline comments.
llvm/tools/llvm-objcopy/CopyConfig.cpp
279 ↗(On Diff #204398)

I think it would be nicer to use a tuple with an enumeration to give the values names rather than "first" and "second"

seiya marked an inline comment as done.Jun 12 2019, 8:47 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/CopyConfig.cpp
279 ↗(On Diff #204398)

What is an elegant way to get an element from tuple using enum? We need to cast the enum values so it should be std::get<static_cast<std::size_t>(Foo::Bar)>(Tuple). It looks verbose a bit to me.

Could you explain a bit more the motivation behind this change, please? If I'm not mistaken, it's to allow using input and output format with non-ELF (e.g. COFF and Mach-O) targets?

llvm/tools/llvm-objcopy/CopyConfig.cpp
279 ↗(On Diff #204398)

I think it would be nicer to use a tuple with an enumeration to give the values names rather than "first" and "second"

Shouldn't it should just be a struct then?

struct {
  FileFormat Format;
  MachineInfo Machine;
};
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
32 ↗(On Diff #204398)

Why has this been added?

135 ↗(On Diff #204398)

Refer -> See

seiya added a comment.Jun 13 2019, 3:30 AM

Could you explain a bit more the motivation behind this change, please? If I'm not mistaken, it's to allow using input and output format with non-ELF (e.g. COFF and Mach-O) targets?

Yes, that's correct. I'd like to use FileFormat instead of a target string for Config.OutputFormat to allow calling non-ELF implementations in executeObjcopyOnRawBinary.

seiya marked an inline comment as done.Jun 13 2019, 3:31 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/CopyConfig.cpp
279 ↗(On Diff #204398)

struct sounds good to me. I'll use it here.

seiya updated this revision to Diff 204475.Jun 13 2019, 3:42 AM
seiya marked 5 inline comments as done.
  • Replaced std::pair with a struct.
  • Refer -> See
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
32 ↗(On Diff #204398)

Oh I forgot to remove this include. Thanks.

jhenderson added inline comments.Jun 13 2019, 8:32 AM
llvm/tools/llvm-objcopy/CopyConfig.h
53 ↗(On Diff #204475)

As this is only used in the .cpp, it can be moved in there.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
134 ↗(On Diff #204475)

Missing trailing full stop.

seiya updated this revision to Diff 204664.Jun 13 2019, 4:57 PM
seiya marked 2 inline comments as done.
  • Addressed review comments.
  • Resolved conflicts with origin/master.
seiya updated this revision to Diff 204665.Jun 13 2019, 4:59 PM
  • Removed a temprary file accidentally included in the patch.

LGTM, with one nit. Also, if I understand it correctly, this change should have no functional change, right? In that case, please tag the summary with [NFC] (i.e. [llvm-objcopy][NFC] Refactor output target parsing) when you commit.

llvm/tools/llvm-objcopy/CopyConfig.cpp
324 ↗(On Diff #204665)

I don't think you need to use auto here.

seiya updated this revision to Diff 204734.Jun 14 2019, 3:25 AM
  • Explicate the type instead of auto.
seiya marked an inline comment as done.Jun 14 2019, 3:25 AM

LGTM, with one nit. Also, if I understand it correctly, this change should have no functional change, right? In that case, please tag the summary with [NFC] (i.e. [llvm-objcopy][NFC] Refactor output target parsing) when you commit.

Yes it is NFC. Thanks.

seiya retitled this revision from [llvm-objcopy] Refactor output target parsing to [llvm-objcopy][NFC] Refactor output target parsing.Jun 14 2019, 3:25 AM
rupprecht added inline comments.Jun 17 2019, 11:04 AM
llvm/tools/llvm-objcopy/CopyConfig.cpp
288 ↗(On Diff #204734)

I haven't looked at the full list of bfd targets (for macho/coff in particular), but do we need the extra field, or can we infer it from starting with "elf"?

457–462 ↗(On Diff #204734)

This should use a StringSwitch

(I suppose adding an Unrecognized file format type to handle the 'else' part?)

473–475 ↗(On Diff #204734)

I'm not sure this is really an error. Behold:

$ echo 'int x=0;' | ~/dev/clang -x c -c - -o /tmp/bss.o
$ objcopy -Ielf32-x86-64 /tmp/bss.o /tmp/bss2.o
$ file /tmp/bss.o /tmp/bss2.o
/tmp/bss.o:  ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped
/tmp/bss2.o: ELF 32-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped
seiya marked 2 inline comments as done.Jun 17 2019, 6:15 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/CopyConfig.cpp
288 ↗(On Diff #204734)

I'm not quite sure but according to my GNU objcopy build (with --enable-targets=all) you're right: we can infer the file type from the prefix.

473–475 ↗(On Diff #204734)

Nice catch! Will fix this.

seiya updated this revision to Diff 205232.Jun 17 2019, 7:09 PM
  • Addressed some review comments.
seiya updated this revision to Diff 205235.Jun 17 2019, 7:25 PM
seiya marked 4 inline comments as done.
  • Refactored a bit.
llvm/tools/llvm-objcopy/CopyConfig.cpp
457–462 ↗(On Diff #204734)

I think we don't need Unrecognized for now because currently we simply ignore unrecognized input formats and guess the file format by createBinary.

jhenderson added inline comments.Jun 18 2019, 3:19 AM
llvm/tools/llvm-objcopy/CopyConfig.cpp
471 ↗(On Diff #205235)

ignores -> ignore

I'm not really sure what this is trying to point out though. Is it an existing bug in llvm-objcopy or one you are introducing?

rupprecht accepted this revision.Jun 18 2019, 11:22 AM
rupprecht marked an inline comment as done.
rupprecht added inline comments.
llvm/tools/llvm-objcopy/CopyConfig.cpp
471 ↗(On Diff #205235)

It's an existing bug; we handle "-I binary" and "-I ihex", but specifying "-Ielf32..." is the same as not specifying it at all.

Maybe a clearer message would be "ignore the target for non-binary/ihex formats"

This revision is now accepted and ready to land.Jun 18 2019, 11:22 AM
seiya updated this revision to Diff 205489.Jun 18 2019, 6:20 PM
seiya marked an inline comment as done.
  • Updated a comment.
jhenderson accepted this revision.Jun 19 2019, 4:25 AM

LGTM, with one comment fix.

llvm/tools/llvm-objcopy/CopyConfig.cpp
471 ↗(On Diff #205489)

Nit: We -> we

seiya updated this revision to Diff 205732.Jun 19 2019, 6:53 PM
  • We -> we
seiya marked an inline comment as done.Jun 19 2019, 6:53 PM

General idea looks good to me!

This revision was automatically updated to reflect the committed changes.