[PATCH] D112291: [lld-macho] Implement -oso_prefix

[Vincent Lee via Phabricator via llvm-commits]

thevinster added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1100
+  if (!config->osoPrefix.empty()) {
+    // Expand "." and "~", if present.
+    SmallString<128> expanded;
----------------
I wasn't able to find where ld64 did this transformation. In fact, I don't see ld64 doing any transformation for the prefix. It looks like it just strips from the debug path if it has the raw prefix. That said, it seems nice to have this extra functionality for LLD. Do you know if real_path also expands `..` as well?


================
Comment at: lld/MachO/Driver.cpp:1101
+    // Expand "." and "~", if present.
+    SmallString<128> expanded;
+    if (!llvm::sys::fs::real_path(config->osoPrefix, expanded,
----------------
Just wondering, how did you come up with 128? Realistically speaking, I haven't seen prefixes that will that much space, but I guess it's possible to have really ugly nested directories on some systems out there....


================
Comment at: lld/MachO/Driver.cpp:1102
+    SmallString<128> expanded;
+    if (!llvm::sys::fs::real_path(config->osoPrefix, expanded,
+                                  /*expand_tilde=*/true)) {
----------------
nit: You can drop the `llvm::sys::`. It seems to be namespaced already.


================
Comment at: lld/MachO/SyntheticSections.cpp:864
+  StringRef adjustedPath = saver.save(path.str());
+  if (!config->osoPrefix.empty())
+    adjustedPath.consume_front(config->osoPrefix);
----------------
Even if it were empty, wouldn't calling `consume_front` be harmless?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112291