This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML/obj2yaml/yaml2obj][MachO] Support indirect symbol table
ClosedPublic

Authored by thevinster on Nov 22 2021, 7:58 PM.

Details

Summary

Tools such as llvm-objdump or llvm-readobj support indirect symbol
tables. Here, support it for obj2yaml and yaml2obj.

Diff Detail

Event Timeline

thevinster created this revision.Nov 22 2021, 7:58 PM

clang-format

thevinster retitled this revision from [obj2yaml/yaml2obj/ObjectYAML][MachO] Support indirect symbol table to [ObjectYAML/obj2yaml/yaml2obj][MachO] Support indirect symbol table.Nov 22 2021, 8:03 PM
thevinster edited the summary of this revision. (Show Details)
thevinster published this revision for review.Nov 22 2021, 8:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2021, 8:06 PM
drodriguez added inline comments.Nov 23 2021, 10:26 AM
llvm/lib/ObjectYAML/MachOEmitter.cpp
568

If I am not wrong, llvm::yaml::Hex32 should have the same size as an uint32_t. In any case, it might be better to use a sizeof(llvm::yaml::Hex32::BaseType) or similar instead of a naked 4 in here.

thevinster marked an inline comment as done.Nov 23 2021, 8:22 PM
thevinster added inline comments.
llvm/lib/ObjectYAML/MachOEmitter.cpp
568

I like sizeof(llvm::yaml::Hex32::BaseType) since it's clearer.

thevinster marked an inline comment as done.
thevinster edited the summary of this revision. (Show Details)

Update to use sizeof() instead of hard-coded number

drodriguez accepted this revision.Nov 29 2021, 10:26 AM
This revision is now accepted and ready to land.Nov 29 2021, 10:26 AM
jhenderson accepted this revision.Nov 30 2021, 12:12 AM

A few nits, but mostly looks good.

llvm/include/llvm/ObjectYAML/MachOYAML.h
124

Shouldn't need the explicit llvm prefix, since you're already in the llvm namespace.

llvm/lib/ObjectYAML/MachOEmitter.cpp
567

UpperCamelCase for variable names.

Also, I'd not use auto here: the type isn't obvious from the RHS (although if you're keen on following existing style in this immediate area, I can live with it).

569
llvm/test/ObjectYAML/MachO/dsymtab.yaml
4

Is this the minimum this document can be reduced to? There's a lot of stuff that has no value to the test, although I know that Mach-O yaml2obj's side doesn't allow much to be removed to minimise noise.

thevinster marked 4 inline comments as done.Nov 30 2021, 3:03 PM
thevinster added inline comments.
llvm/test/ObjectYAML/MachO/dsymtab.yaml
4

Updated. Most of the LinkEditData could be removed (except the one under test), but the FileHeader and LoadCommands are required in order to not break the continuity of the binary.

thevinster marked an inline comment as done.

Addressing nits

This revision was landed with ongoing or failed builds.Nov 30 2021, 4:15 PM
This revision was automatically updated to reflect the committed changes.