This is an archive of the discontinued LLVM Phabricator instance.

[llvm][llvm-nm] add TextAPI/MachO support
ClosedPublic

Authored by cishida on Jun 10 2020, 2:58 PM.

Details

Summary

This completes the needed glueing to support reading tbd files from nm.
This includes specifying which slice filtering with --arch and a new
option specifically for tbd files --add-inlinedinfo which will show
the reexported libraries that are appended in the tbd file.

Diff Detail

Event Timeline

cishida created this revision.Jun 10 2020, 2:58 PM
JDevlieghere added inline comments.Jun 10 2020, 3:07 PM
llvm/include/llvm/Object/TapiFile.h
57

Why not just Arch like the ctor argument?

llvm/include/llvm/TextAPI/MachO/Architecture.h
50

Would getNumberOfBits be a more "generic" API?

llvm/lib/Object/TapiFile.cpp
44

Why not do this in the initializer list?

llvm/lib/TextAPI/MachO/Architecture.cpp
91

llvm_unreachable("Fully covered switch above!");

llvm/tools/llvm-nm/llvm-nm.cpp
2111

I think you should be able to do:

if (auto ObjOrErr = I.getAsObjectFile())
cishida marked 4 inline comments as done.Jun 10 2020, 3:50 PM
cishida added inline comments.
llvm/include/llvm/TextAPI/MachO/Architecture.h
50

I agree but I named that to conform to the MachO api.

JDevlieghere marked an inline comment as done.Jun 10 2020, 3:52 PM
JDevlieghere added inline comments.
llvm/include/llvm/TextAPI/MachO/Architecture.h
50

Fair enough.

cishida updated this revision to Diff 269987.Jun 10 2020, 3:58 PM

Updating to address comments, add more const correctness, fix casing on FileCheck

I don't know anything about the tapi/tbd stuff, to speak of, so I can't give any concrete comments, just a number of nits.

llvm/include/llvm/Object/TapiUniversal.h
57

Nit: missing blank line between this and the previous function.

llvm/include/llvm/TextAPI/MachO/Architecture.h
49

Nit: missing full stop.

llvm/lib/Object/TapiUniversal.cpp
33–43

I see auto was being used before in various places, but I'm not a fan where it's not clear what the type is (which is the case in almost every instance in these two blocks). However, I'm not familiar with this area of the code base (i.e. the tapi stuff), so feel free to ignore me if this is common style in this area.

llvm/tools/llvm-nm/llvm-nm.cpp
2101

Same comment as earlier - don't use auto where the type is not obvious from the immediate context.

2105

Example why auto is bad: I'm assuming Name is actually a StringRef, in which case, you don't need the const & part of this, since StringRef is designed for lightweight copying.

cishida marked 7 inline comments as done.Jun 11 2020, 8:34 AM
cishida updated this revision to Diff 270154.Jun 11 2020, 8:35 AM

adopintg jhenderson's comments to increase Readablity & Coding Guidelines

This revision is now accepted and ready to land.Jun 11 2020, 9:46 AM
MaskRay added inline comments.Jun 11 2020, 11:54 AM
llvm/tools/llvm-nm/llvm-nm.cpp
2111

Obj is only used once. Inline the variable to the call site

2116

auto -> Error

MaskRay added inline comments.Jun 11 2020, 11:56 AM
llvm/test/Object/Inputs/tapi-invalid-v2.tbd
1

Consider moving new test/Object/* tests to test/Object/MachO/*

cishida marked 3 inline comments as done.Jun 11 2020, 6:52 PM

the landed patch has applied MaskRay's comments.

llvm/test/Object/Inputs/tapi-invalid-v2.tbd
1

Since that directory doesn't exist in the repo currently, I will land a separate NFC commit to move all the machO based nm tests there.

This revision was automatically updated to reflect the committed changes.