Page MenuHomePhabricator

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

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

Details

Summary

Bug: https://bugs.llvm.org/show_bug.cgi?id=49278
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
lld/MachO/Driver.cpp
822

no need for llvm::

824
lld/MachO/Driver.h
71–72

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

93

LLVM tries to avoid std::set (and std::map): https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc

In this case we could use a DenseSet

115–120

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.

lld/MachO/DriverUtils.cpp
263

legit lint

272

no need for llvm::

295

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

299

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

lld/test/MachO/Inputs/DependencyDump.py
5

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

lld/test/MachO/dependency-info.s
31

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

lld/MachO/Driver.h
93

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.
lld/MachO/Driver.cpp
57–59

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

821–831

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

lld/MachO/Driver.h
91–93

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)

93

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

106–108

rm?

lld/MachO/DriverUtils.cpp
253

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
lld/MachO/Driver.cpp
825

extra parens

826

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

lld/MachO/Driver.h
91–93

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 http://45.33.8.238/linux/42252/step_11.txt

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

Also doesn't build on win and mac:
http://45.33.8.238/win/35446/step_4.txt
http://45.33.8.238/macm1/5982/step_3.txt

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

thakis added inline comments.Mar 21 2021, 1:31 PM
lld/test/MachO/dependency-info.s
27

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.Mon, Mar 22, 10:20 AM
This revision is now accepted and ready to land.Mon, Mar 22, 10:20 AM
oontvoo updated this revision to Diff 332352.Mon, Mar 22, 10:20 AM

moved inline functions to header

oontvoo added inline comments.Mon, Mar 22, 11:04 AM
lld/test/MachO/dependency-info.s
27

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.Mon, Mar 22, 11:41 AM

updated diff

int3 added inline comments.Mon, Mar 22, 12:08 PM
lld/test/MachO/dependency-info.s
27

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.Mon, Mar 22, 2:06 PM
oontvoo marked an inline comment as done.
This revision is now accepted and ready to land.Mon, Mar 22, 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;
    ~~ ^  ~~~~~~

https://buildkite.com/mlir/mlir-core/builds/12456#920bd17a-4c37-435c-b91d-aca1cc15400b

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

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

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

oontvoo updated this revision to Diff 332708.Tue, Mar 23, 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: https://gcc.godbolt.org/z/bT6zE7x77

oontvoo closed this revision.Tue, Mar 23, 11:54 AM
int3 added inline comments.Tue, Mar 23, 12:30 PM
lld/MachO/DriverUtils.cpp
287–288

I think this does the same thing?

lld/test/MachO/dependency-info.s
2–4

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

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

I've sent D99210