Details
- Reviewers
phosek jhenderson - Commits
- rGa45afd50d459: Reland "[llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr"
rGe5d424b8dccf: Reland "[llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr"
rGf20c3f4333ae: [llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr
rL313772: Reland "[llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr"
rL313767: Reland "[llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr"
rL313663: [llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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.
- Added and used getSection methods to help with problem of constantly needed to access and error check sections
- Used SectionWithStrTab change recomended
- Fixed typos
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. |
Please also check the section header data like in the other tests.