Page MenuHomePhabricator

[llvm-readobj] Support -needed-libs option for Mach-O files
ClosedPublic

Authored by phosek on Dec 21 2017, 3:56 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Dec 21 2017, 3:56 PM

To give more context, I'm planning on implementing this feature for COFF as well and then using llvm-readobj in D41272 to have a solution that works across all platforms.

davide requested changes to this revision.Dec 21 2017, 8:15 PM

Can you please add a testcase, Petr?

This revision now requires changes to proceed.Dec 21 2017, 8:15 PM

@davide Is there anything else?

davide requested changes to this revision.Dec 22 2017, 12:49 PM

I'm across the pond fo the rest of the year, sorry if communication is a little slow :)

tools/llvm-readobj/MachODumper.cpp
42–43 ↗(On Diff #127991)

this is Mach-O specific no? So it should be grouped with the others.

687–692 ↗(On Diff #127991)

maybe this would be more readable if it used a switch, not sure if it's entirely feasible.

695 ↗(On Diff #127991)

static_cast<> and auto *.

701 ↗(On Diff #127991)

Why you need this stable sort?

This revision now requires changes to proceed.Dec 22 2017, 12:49 PM
phosek updated this revision to Diff 128054.Dec 22 2017, 1:01 PM
phosek marked an inline comment as done.
phosek added inline comments.
tools/llvm-readobj/MachODumper.cpp
42–43 ↗(On Diff #127991)

This should be implemented for all formats, see https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ObjDumper.h#L42. It's currently implemented for ELF, I'm trying to add an implementation for Mach-O (D41527) and COFF (D41529).

701 ↗(On Diff #127991)

I have followed the ELF implementation which also uses stable sort, I'm not sure why it's used there.

davide accepted this revision.Dec 22 2017, 1:06 PM

LGTM with one comment addressed. Thanks.

tools/llvm-readobj/MachODumper.cpp
701 ↗(On Diff #127991)

I'd rather not cargo cult this here unless we have a good reason to. We shouldn't need this call, should we?

This revision is now accepted and ready to land.Dec 22 2017, 1:06 PM
phosek added inline comments.Dec 22 2017, 1:09 PM
tools/llvm-readobj/MachODumper.cpp
701 ↗(On Diff #127991)

I think having a stable output order may be useful in some cases. Also if that's what we're already doing for ELF version, we should probably be consistent across all formats?

davide added inline comments.Dec 23 2017, 5:40 AM
tools/llvm-readobj/MachODumper.cpp
701 ↗(On Diff #127991)

I think we should have a stable order of these *regardless*. I'm nervous about adding a call to std::stable_sort() unless we have a good reason for it, because it might hide bugs elsewhere in the reader.

phosek added inline comments.Dec 27 2017, 12:31 PM
tools/llvm-readobj/MachODumper.cpp
701 ↗(On Diff #127991)

Understood, the problem is that load_command() doesn't provide any guarantees about the ordering, the order should be the same as the order in which the commands were read from the file, but commands are also not guaranteed to be sorted and after briefly checking a few binaries they definitely aren't. So the way to guarantee ordering would be to sort LoadCommands in MachOObjectFile constructor when reading the file, is that what you're suggesting?

This revision was automatically updated to reflect the committed changes.