[PATCH] D98559: [lld-macho] Implement -dependency_info

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:821
+  if (const Arg *arg = args.getLastArg(OPT_dependency_info)) {
+    llvm::StringRef path = arg->getValue();
+    if (path.empty() || (fs::exists(path) && !fs::can_write(path)))
----------------
no need for llvm::


================
Comment at: lld/MachO/Driver.cpp:823
+    if (path.empty() || (fs::exists(path) && !fs::can_write(path)))
+      warn("Ignore dependency_info option since specified path is not "
+           "writeable.");
----------------



================
Comment at: lld/MachO/Driver.h:71-72
+public:
+  static void instantiate(llvm::StringRef path);
+  static DependencyTracker *instance();
+
----------------
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


================
Comment at: lld/MachO/Driver.h:93
+  // constructed.
+  std::set<std::string> notFounds;
+
----------------
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`


================
Comment at: lld/MachO/Driver.h:115-120
+// There are two overloads, with one taking a StringRef because
+// sometimes the temporary paths are constructed as a StringRef and we
+// don't want pay the cost of constructing a full std::string unnecessarily.
+template <typename T,
+          typename = std::enable_if<false, std::is_same<T, std::string>>>
+void inline logFileNotFound(T path) {
----------------
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.


================
Comment at: lld/MachO/DriverUtils.cpp:263
+
+  auto AddDep = [&os](DepOpCode opcode, const llvm::StringRef &path) {
+    os << opcode;
----------------
legit lint


================
Comment at: lld/MachO/DriverUtils.cpp:272
+  // Sort the input by its names.
+  std::vector<llvm::StringRef> inputNames;
+  inputNames.reserve(inputs.size());
----------------
no need for llvm::


================
Comment at: lld/MachO/DriverUtils.cpp:295
+    return;
+  singleton = new (getSpecificAllocSingleton<DependencyTracker>().Allocate())
+      DependencyTracker(path);
----------------
fwiw, the other LLD ports don't use `getSpecificAllocSingleton` at all... I wonder if it's really that useful an optimization 🤔


================
Comment at: lld/MachO/DriverUtils.cpp:299
+
+DependencyTracker::~DependencyTracker() { notFounds.clear(); }
+
----------------
this isn't necessary, the default `DependencyTracker` destructor will call the std::set destructor, which will free this memory


================
Comment at: lld/test/MachO/Inputs/DependencyDump.py:4
+# format for testing purposes.
+# This is copied from lld/test/mach-o/Inputs/DependencyDump.py
+#
----------------
the old LLD for Mach-O code should be going away soon, let's not reference it


================
Comment at: lld/test/MachO/dependency-info.s:30
+# CHECK-NEXT: not-found: {{.*}}/libdyld.dylib
+# There could be more not-found here but we are not checking those because it's brittle.
+
----------------
convention is to double-comment lines that aren't passed to FileCheck


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98559/new/

https://reviews.llvm.org/D98559