This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][ELF] Add --set-section-type
ClosedPublic

Authored by MaskRay on Jul 7 2022, 5:28 PM.

Details

Summary

The request is mentioned on D129053. I feel that having this functionality is
mildly useful (not strong).

  • Rename .ctors to .init_array and change sh_type to SHT_INIT_ARRAY (GNU objcopy detects the special name but we don't).
  • Craft tests for a new SHT_LLVM_* extension

Diff Detail

Event Timeline

MaskRay created this revision.Jul 7 2022, 5:28 PM
MaskRay requested review of this revision.Jul 7 2022, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2022, 5:28 PM
MaskRay updated this revision to Diff 443096.Jul 7 2022, 5:34 PM

more test

Documentation needs updating.

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
686–688

As --set-section-flags can potentially change the type, we might want to add a specific test to document the behaviour that --set-section-type overrides whatever --set-section-flags does.

llvm/test/tools/llvm-objcopy/ELF/set-section-attr-and-rename.test
15–21

Would it not make sense to fold this into the above test-case, so that it tests all three of the --set-section-* options in one go?

llvm/test/tools/llvm-objcopy/ELF/set-section-type.test
21

I pronounce uint32_t as "you-int-thir-ty-two-tee", which would mean it should be "a" not "an". I'm curious if you say it differently?

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
279

I think you should be able to avoid the code duplication with parseSetSectionAlignment by factoring most of this code out into a separate function called both here and there.

289

If not refactoring, this local variable name is wrong.

811

Local variable needs renaming.

828–832

As this is quite verbose, it might be good to factor it into a helper function or lambda, to avoid duplication with the set section flags case.

MaskRay updated this revision to Diff 443835.Jul 11 2022, 10:26 PM
MaskRay marked 7 inline comments as done.

Thanks for the comments. Updated.

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
686–688

Added a test ## --set-section-flags does not specify "readonly", so the output gets SHF_WRITE.

llvm/test/tools/llvm-objcopy/ELF/set-section-type.test
21

I say "you-int-thir-ty-two-tee", too... Fixed.

jhenderson added inline comments.Jul 12 2022, 1:06 AM
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
241–242

I'm not sure I follow the reasoning for switch to make_error?

818
MaskRay updated this revision to Diff 443998.Jul 12 2022, 10:07 AM
MaskRay marked 2 inline comments as done.

update a comment

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
241–242

createStringError is a wrapper which accepts const char *. The underlying StringError constructor actually supports Twine.

jhenderson accepted this revision.Jul 13 2022, 12:18 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 13 2022, 12:18 AM
jhenderson requested changes to this revision.Jul 13 2022, 12:18 AM

Hang on, looks like the llvm-objcopy CommandGuide file hasn't been updated?

This revision now requires changes to proceed.Jul 13 2022, 12:18 AM
MaskRay updated this revision to Diff 444172.Jul 13 2022, 12:30 AM

update llvm/docs/CommandGuide/llvm-objcopy.rst

jhenderson accepted this revision.Jul 13 2022, 12:45 AM

LGTM, with suggested changes.

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

Unrelated, but if you get a moment whilst updating this doc, would you mean fixing the typo with the uneven ` characters around "<align>", please?

174 ↗(On Diff #444172)

I suggest adopting the wording that --set-section-alignment uses. Something like this would be good:

Set the type of section ``<section>`` to the integer ``<type>``. Can be specified multiple times to update multiple sections.

As this option is ELF-only, it should be put under the "ELF-SPECIFIC OPTIONS" section of the doc.

This revision is now accepted and ready to land.Jul 13 2022, 12:45 AM
This revision was landed with ongoing or failed builds.Jul 13 2022, 10:04 AM
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.
MaskRay marked an inline comment as done.Jul 13 2022, 10:14 AM
MaskRay added inline comments.
llvm/docs/CommandGuide/llvm-objcopy.rst
130 ↗(On Diff #444172)

Thanks for spotting this! Fixed separately in ccfc54028860a71780361128f861444825a24a05

174 ↗(On Diff #444172)

Thanks for the suggestion. Moved and updated the wording.

MaskRay marked an inline comment as done.Jul 13 2022, 10:15 AM
llvm/test/tools/llvm-objcopy/ELF/add-section-and-set-attr.test