This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr
ClosedPublic

Authored by jakehehrlich on Aug 9 2017, 4:49 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jakehehrlich created this revision.Aug 9 2017, 4:49 PM
jhenderson edited edge metadata.Aug 30 2017, 2:01 AM

I take it yaml2obj cannot create dynamic section/symbol table information? If it could, it would mean that no pre-built binaries would be needed for the tests.

A thought on the duplication of DynamicSymbolTableSection and DynamicSection: what would you think of having a shared class called something like "SectionWithStringTable", and then implementing these two section types as inheriting from that one. The only logic needed in DynamicSection/DynamicSymbolTableSection would then be the classof method.

test/tools/llvm-objcopy/dynsym.test
4

Please also check the section header data like in the other tests.

tools/llvm-objcopy/Object.cpp
406

Full stop.

415

Ditto. I'd also replace the last word "anyway" with "either".

423

DynamicSection not DynamicSymbolTableSection!

486

As with other cases, we need to check that the Link value is actually a valid section index, or we will get a crash. Since this is a recurring theme, perhaps Object should gain a getSection() method that does the error checking?

490

Ditto.

tools/llvm-objcopy/Object.h
197–200

As the DynamicSection class currently appears after this class, I think that either a) the comment should be moved to the second class to appear in order (i.e. DynamicSection), or b) the DynamicSection should be moved to before the DynamicSymbolTableSection. This means that the comment refers to something that a user reading top-down would have already read about.

I take it yaml2obj cannot create dynamic section/symbol table information? If it could, it would mean that no pre-built binaries would be needed for the tests.

This is correct. It might not be so hard to add though. It supports just

A thought on the duplication of DynamicSymbolTableSection and DynamicSection: what would you think of having a shared class called something like "SectionWithStringTable", and then implementing these two section types as inheriting from that one. The only logic needed in DynamicSection/DynamicSymbolTableSection would then be the classof method.

I like that. I'll upload that in the next diff when I fix these other changes.

  1. Added and used getSection methods to help with problem of constantly needed to access and error check sections
  2. Used SectionWithStrTab change recomended
  3. Fixed typos
jhenderson added inline comments.Sep 18 2017, 6:18 AM
tools/llvm-objcopy/Object.cpp
398

speical -> special

tools/llvm-objcopy/Object.h
210

Has this been clang-formatted? The line looks a bit long.

244

Not really important, but I'm not a massive fan of this name - maybe getSectionOfType?

  1. Changed name of method to getSectionOfType
  2. Fixed typos
jhenderson accepted this revision.Sep 19 2017, 1:35 AM

LGTM with the suggested argument name changes.

tools/llvm-objcopy/Object.cpp
381

The last argument name now needs changing both here and in the header. It may be wise to change ErrMsg as well to clearly disambiguate the two. Suggestions: ErrMsg -> IndexErrMsg; CastErrMsg -> TypeErrMsg.

This revision is now accepted and ready to land.Sep 19 2017, 1:35 AM
This revision was automatically updated to reflect the committed changes.