Page MenuHomePhabricator

[llvm-readobj][MachO] Fix section type printing
ClosedPublic

Authored by seiya on Aug 12 2019, 1:59 AM.

Details

Summary

Currently, llvm-readobj mistakenly decodes section type as section attribute.

This patch fixes the bug and affected tests.

Diff Detail

Repository
rL LLVM

Event Timeline

seiya created this revision.Aug 12 2019, 1:59 AM
seiya edited the summary of this revision. (Show Details)Aug 12 2019, 1:59 AM
seiya added reviewers: jhenderson, rupprecht, alexshap.

The change looks fine to me, although I wasn't able to quickly find the list of section types anywhere to check against. Could you provide me some documentation?

Also, are there test cases for each of the different permitted types?

The change looks fine to me, although I wasn't able to quickly find the list of section types anywhere to check against. Could you provide me some documentation?

This was already committed to LLVM years ago in rL178679 (+echristo, the committer), but mistakenly removed in rL235863 due to being an unused variable, instead of the right fix, which is clearly the Attributes->Types typo fixed here

Still, I can't find a reference for the original list, it would be good to cite one.

Also, are there test cases for each of the different permitted types?

+1, would be nice to have a test in llvm-readobj that enumerates all section types in a yaml2obj case

(otherwise, LGTM)

echristo accepted this revision.Aug 12 2019, 2:43 PM

The list looks about right from what I recall - we should get a link to where they're documented, but the Apple documentation site moves a bit more than I'd like so this might be the best we can do for now.

This revision is now accepted and ready to land.Aug 12 2019, 2:43 PM
seiya added a comment.EditedAug 12 2019, 8:30 PM

Thank you for pointing out the related commits, @rupprecht!

The change looks fine to me, although I wasn't able to quickly find the list of section types anywhere to check against. Could you provide me some documentation?

A list of section types in mach-o/loader.h (in Apple's binary utilities) or llvm/Binary/Format/MachO.h might be helpful.

Also, are there test cases for each of the different permitted types?

It looks few of them are not used in these tests. I'll add a new testcase.

seiya updated this revision to Diff 214759.Aug 12 2019, 10:22 PM

Add llvm/test/tools/llvm-readobj/macho-sections.test, which exhaustively tests all section types.

rupprecht accepted this revision.Aug 12 2019, 10:33 PM
jhenderson added inline comments.Aug 13 2019, 6:57 AM
llvm/test/tools/llvm-readobj/macho-sections.test
51 ↗(On Diff #214759)

Are all fields required, or can some be omitted? Same goes throughout the YAML.

llvm/tools/llvm-readobj/MachODumper.cpp
218 ↗(On Diff #214759)

Since they exist, you should refer to the values in SectionType enum in MachO.h.

227–228 ↗(On Diff #214759)

ModInitFuncPointers and ModTermFuncPointers would make more sense.

234 ↗(On Diff #214759)

LazyDylibSymbolPointers

seiya updated this revision to Diff 215001.Aug 13 2019, 6:16 PM
seiya marked 3 inline comments as done.
  • Addressed review comments.
    • Replaced magic numbers with enum MachO::SectionType.
    • Renamed some section types.
seiya marked 2 inline comments as done.Aug 13 2019, 6:22 PM
seiya added inline comments.
llvm/test/tools/llvm-readobj/macho-sections.test
51 ↗(On Diff #214759)

Yes they are. (I was wondering about that the whole time and I'd like to make some of them optional at some time if possible)

alexshap accepted this revision.Aug 14 2019, 5:31 AM

LG, thanks!

jhenderson accepted this revision.Aug 14 2019, 6:02 AM

LGTM too.

This revision was automatically updated to reflect the committed changes.
seiya marked an inline comment as done.