This is an archive of the discontinued LLVM Phabricator instance.

[docs][llvm-objcopy] Write documentation for llvm-objcopy
ClosedPublic

Authored by jhenderson on Jun 26 2019, 6:57 AM.

Details

Summary

This patch addresses https://bugs.llvm.org/show_bug.cgi?id=42183 by replacing the stub markdown doc for llvm-objcopy with a full one describing the current options available in llvm-objcopy.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Jun 26 2019, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 6:57 AM
MaskRay added inline comments.Jun 27 2019, 2:49 AM
docs/CommandGuide/llvm-objcopy.rst
418 ↗(On Diff #206653)

SUPPORTED FORMATS might be a bit more appropriate as the section header.

It may be worth mentioning elf* below are bfdnames.

  • Change SUPPORTED TARGETS to SUPPORTED FORMATS.
  • Add a note about the supported formats being bfdnames for compatibility.
  • Normalized some wording for the --.*target options.
  • Added a .. program directive to ensure that option links remain pointing to the right document.
jhenderson marked an inline comment as done.Jul 1 2019, 4:43 AM

This is *awesome*. This is much more clear and precise that the GNU objcopy documentation!

I hadn't realized just how far the tool has come! Great job!

docs/CommandGuide/llvm-objcopy.rst
81–82 ↗(On Diff #206814)

We keep .gnu.warning sections as well right?

130 ↗(On Diff #206814)

This is half fair. Technically it meets the requirements for ELF it just doesn't work optimally. I think adding a caveat section under ELF-specific section might be warranted.

226 ↗(On Diff #206814)

Might add "with local binding"

379 ↗(On Diff #206814)

Say "with no other arguments" to clarify that the --extract-dwo file has no flags applied to it.

394 ↗(On Diff #206814)

I'd note that many non-llvm tools are incompatible with this option and that it should only be used when a) you need the absolute smallest file possible and b) should never be used on ET_REL binaries.

MaskRay added inline comments.Jul 2 2019, 12:27 AM
docs/CommandGuide/llvm-objcopy.rst
9 ↗(On Diff #206814)

It may be more common to place options before positional arguments, i.e.

:program:`llvm-objcopy` [*options*] *input* [*output*]
jhenderson updated this revision to Diff 207513.Jul 2 2019, 4:28 AM
jhenderson marked 6 inline comments as done.

Address review comments.

docs/CommandGuide/llvm-objcopy.rst
130 ↗(On Diff #206814)

Config.OnlyKeepDebug is not used outside the COFF code, so this really is ignored for ELF as things stand. Given that the output of using this on ELF is not what GNU produces (i.e. it's a no-op), I'm not convinced we should advertise this more widely. We could make similar arguments about various options that are currently ELF-specific, and aren't checked by the COFF code for an error.

I've added a blanket comment to the top of the section saying that llvm-objcopy will either ignore or error for format-specific options (same for ELF).

226 ↗(On Diff #206814)

Isn't that what "local symbols" means?

394 ↗(On Diff #206814)

I've added a more general note that "many tools will not be able to" handle such objects.

I don't see a reason to mention when you should or shouldn't do this beyond that statement because ultimately if a user is using this switch, they will know why they are using it, whether it be for size, obfuscation, or whatever. Telling people that it's always about size is like telling them what their motivation is.

If it's run on an ET_REL binary, then the entire section contents of the file will be removed. I don't think this needs explicitly stating, since that can be inferred from the reference to "not within segments".

jhenderson updated this revision to Diff 207550.Jul 2 2019, 6:42 AM

Improve text of --dump-section to clarify that the regular copying still takes place.

jakehehrlich marked an inline comment as done.Jul 2 2019, 2:39 PM

LGTM (sorry a chrome bug is preventing me from using drop down menus...sigh)

docs/CommandGuide/llvm-objcopy.rst
232 ↗(On Diff #207550)

Perhaps its worth mentioning that operations are not performed on the section itself prior to dumping?

jakehehrlich accepted this revision.Jul 2 2019, 2:39 PM

(fixed it)

This revision is now accepted and ready to land.Jul 2 2019, 2:39 PM
MaskRay accepted this revision.Jul 2 2019, 8:33 PM
This revision was automatically updated to reflect the committed changes.