Page MenuHomePhabricator

[lld-macho] Implement -dependency_info (partially - more opcodes needed)

Authored by oontvoo on Mar 12 2021, 2:48 PM.



The flag is not well documented, so this implementation is based on observed behaviour.

When specified, -dependency_info <path> produced a text file containing information pertaining to the current linkage, such as input files, output file, linker version, etc.

This file's layout is also not documented, but it seems to be a series of null ('\0') terminated strings in the form <op code><path>

<op code> could be:

`0x00` : linker version
`0x10` : input
`0x11` : files not found(??)
`0x40` : output

<path> : is the file path, except for the linker-version case.

(??) This part is a bit unclear. I think it means all the files the linker attempted to look at, but could not find.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
oontvoo marked 2 inline comments as done.Mar 15 2021, 2:57 PM

updated diff:

  • addressed review comments
  • added tests
oontvoo updated this revision to Diff 331019.Mar 16 2021, 9:41 AM

Updated test to exclude windows because the paths are in different orders

int3 added inline comments.Mar 17 2021, 3:04 PM

no need for llvm::


This is a very Java-like way of creating singletons :) The rest of the LLD code just uses a global (config, target, inputFiles etc). I think DependencyTracker should follow suit


LLVM tries to avoid std::set (and std::map):

In this case we could use a DenseSet


The number of files will be pretty small, there's no real need to optimize this. That said... could we take a Twine here instead of using templates?

Nit: Also, I'd prefer if this wasn't a global, at least not with such a generic name. Could we always initialize DependencyTracker. and have its methods be no-ops if -dependency_info wasn't passed? Alternatively, just having this named logDepNotFound would be an improvement, at least in that case we have a hint as to why we're logging it.


legit lint


no need for llvm::


fwiw, the other LLD ports don't use getSpecificAllocSingleton at all... I wonder if it's really that useful an optimization 🤔


this isn't necessary, the default DependencyTracker destructor will call the std::set destructor, which will free this memory


the old LLD for Mach-O code should be going away soon, let's not reference it


convention is to double-comment lines that aren't passed to FileCheck

oontvoo planned changes to this revision.Mar 18 2021, 8:14 AM
oontvoo updated this revision to Diff 332000.Mar 19 2021, 2:12 PM
oontvoo marked 10 inline comments as done.

updated diff


I've thought about this, and IIRC DenseSet's doesn't offer the ordering we need.
And I'd rather not have to dump it to a vector and re-sort at the end.
Is there other ds we could use?

int3 accepted this revision.Mar 19 2021, 4:44 PM
int3 added inline comments.

I think we could drop lld:: from both of these


I don't see why this needs to be in a lambda vs just putting it in the constructor of DependencyTracker (and moving the definition of the ctor into DriverUtils.cpp).

Also, args.getLastArgValue(OPT_dependency_info) will return an empty string if the option is not available, so you can skip the null check


re the "weird" comment: I think the purpose of this is so that the build system can re-invoke the linker when any of its input files change (including input files that did not previously exist)


ah I see. yeah I guess std::set is good then




I think we're suppose to pass Twines as const references rather than by value, so that the temporary values they reference don't get freed

This revision is now accepted and ready to land.Mar 19 2021, 4:44 PM
int3 added a comment.Mar 19 2021, 4:45 PM

Also maybe title the diff something like "partially implement" since we have more opcodes to add

oontvoo retitled this revision from [lld-macho] Implement -dependency_info to [lld-macho] Implement -dependency_info (partially - more opcodes needed).Mar 21 2021, 10:35 AM
int3 added inline comments.Mar 21 2021, 10:53 AM

extra parens


our other option-related messages include the dash, so it would be nice to use that here too :)

oontvoo updated this revision to Diff 332168.Mar 21 2021, 11:02 AM
oontvoo marked 6 inline comments as done.

updated diff


Thanks! removed the comment.

oontvoo updated this revision to Diff 332171.Mar 21 2021, 11:35 AM

updated diff

This revision was landed with ongoing or failed builds.Mar 21 2021, 11:36 AM
This revision was automatically updated to reflect the committed changes.

This breaks tests everywhere, eg

Please take a look, and revert for now if it takes a while to fix.

Also doesn't build on win and mac:

(due to the inline on a fun def in a .cpp file)

thakis added inline comments.Mar 21 2021, 1:31 PM

I guess the problem is that these lines are sorted by name and libSystem is in the src tree and the rest are generated files, and the order of the names of build dir and src dir isn't predictable.

oontvoo reopened this revision.Mar 22 2021, 10:20 AM
This revision is now accepted and ready to land.Mar 22 2021, 10:20 AM
oontvoo updated this revision to Diff 332352.Mar 22 2021, 10:20 AM

moved inline functions to header

oontvoo added inline comments.Mar 22 2021, 11:04 AM

Ah, good catch! Hmm, maybe I'll just grep -v libSystem.tbd from the output before we FileCheck?
(Does anyone have a better solution?)

oontvoo updated this revision to Diff 332377.Mar 22 2021, 11:41 AM

updated diff

int3 added inline comments.Mar 22 2021, 12:08 PM

I guess you could just turn all the CHECK-NEXT lines into CHECK-DAG, and leave a comment saying that this is sorted but gives different orders due to the build/src dirs.

(As an aside, I'm not sure having these entries in sorted order is super important... I imagine anything that cares about monitoring dependencies just cares about the unordered set of input files)

oontvoo updated this revision to Diff 332420.Mar 22 2021, 2:06 PM
oontvoo marked an inline comment as done.
This revision is now accepted and ready to land.Mar 22 2021, 2:06 PM

Reverted in 4876ba5b2d6a : the build is broken:

lld/MachO/DriverUtils.cpp:271:8: error: use of overloaded operator '<<' is ambiguous (with operand types 'llvm::raw_fd_ostream' and 'lld::macho::DependencyTracker::DepOpCode')
    os << opcode;
    ~~ ^  ~~~~~~

Thanks for the revert! May be this patch is cursed 😕

oontvoo updated this revision to Diff 332508.Mar 22 2021, 8:26 PM

Updated diff: explicitly specified "signed char" to avoid ambiguity

oontvoo updated this revision to Diff 332708.Mar 23 2021, 9:54 AM

Updated diff. rebase.

So it's turned out that this was likely a clang bug (or unimplemented feature).
In clang older than 10.x, it didn't know to cast the enum (DepOpCode) to its underlying type. Hence, << DepOpCode was ambiguous to it because the enum could be converted to any of the integer types.

Simple repro:

oontvoo closed this revision.Mar 23 2021, 11:54 AM
int3 added inline comments.Mar 23 2021, 12:30 PM

I think this does the same thing?


can this be removed now that we have CHECK-DAG?

oontvoo marked an inline comment as done.Mar 23 2021, 12:55 PM

I've sent D99210