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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1100
+  if (!config->osoPrefix.empty()) {
+    // Expand "." and "~", if present.
+    SmallString<128> expanded;
----------------
int3 wrote:
> thevinster wrote:
> > 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?
> if ld64 isn't doing it, I would prefer we not add extra logic to LLD just because :)
LD64 doesn't expand anything other than the `.`
But IMO, that seems weird to handle only one special notation and not the others .  I think we should just use `real_path` to do it - and if it happens to be able to do more transformations than LD64 then so be it. I can't think of a reason why someone would pass `~`, for eg., and not expect it to be expanded - especially given that the path we emit to stabs never contains these special characters anyway.



================
Comment at: lld/MachO/Driver.cpp:1101
+    // Expand "." and "~", if present.
+    SmallString<128> expanded;
+    if (!llvm::sys::fs::real_path(config->osoPrefix, expanded,
----------------
int3 wrote:
> thevinster wrote:
> > 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....
> We've been using 261 for similar things in the codebase, because the max filename length on Windows is 260. Max filename length on Linux is 255, but max *path* length is 4096, so I guess to be really safe some of these path strings should have 4096 bytes allocated...
The 128 originally came from copy-and-paste - I'd have thought it should be plenty for most cases.

Ok, yakshaving here  :) , but isn't 4096 too much (if the prefix is 4096 ie., max length, then you're trimming everything, no?)
How about 512? (based on slightly unscientific observation that the top 10  longest paths in lld-macho's test output are  253 to 266  chars long, on my machine)


```
 vyng at vyng:/mnt/ssd/repo/llvm-project/build_lld$ ls -R /mnt/ssd/repo/llvm-project/build_lld | awk '
/:$/&&f{s=$0;f=0}
/:$/&&!f{sub(/:$/,"");s=$0;f=1;next}
NF&&f{ print s"/"$0 }' | awk '{print length, $0}' | sort -nr | head -n10
266 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro6/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/libfoo.dylib
266 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro6/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/libbar.dylib
266 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro5/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/libbar.dylib
266 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro4/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/libbar.dylib
266 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro3/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/libbar.dylib
259 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro2/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/bar.a
259 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro1/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/bar.a
253 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro6/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp
253 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro5/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp
253 /mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/repro4/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp/mnt/ssd/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/reroot-path.s.tmp

```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112291