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

Fri Oct 22 11:58:41 PDT 2021

int3 accepted this revision.
int3 added a comment.

lgtm

================
Comment at: lld/MachO/Driver.cpp:1100
+  if (!config->osoPrefix.empty()) {
+    // Expand "." and "~", if present.
+    SmallString<128> expanded;
----------------
oontvoo wrote:
> 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.
> 
Gotcha. Yeah, if we have to handle ".", it makes sense to use `real_path` and handle all the cases. Worth leaving a comment here that ld64 only handles "." though.

================
Comment at: lld/MachO/Driver.cpp:1101
+    // Expand "." and "~", if present.
+    SmallString<128> expanded;
+    if (!llvm::sys::fs::real_path(config->osoPrefix, expanded,
----------------
oontvoo wrote:
> 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
> 
> ```
> 
what's 4k bytes between friends? :p

yeah, looking at the codebase, I see there are a bunch of `SmallString<128>`s, but also 260s and 261s...

feel free to put whatever here; I think I'll codemod them to something more consistent later

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

================
Comment at: lld/test/MachO/stabs.s:76
+# REL-PATH:   (N_OSO        ) 03                         0001   [[#%.16x,TEST_TIME]] '/test.o'
+# REL-PATH-DOT:   (N_OSO        ) 03                         0001   [[#%.16x,TEST_TIME]] 'test.o'        
 # CHECK-NEXT: (N_STSYM      ) [[#%.2d,MORE_DATA_ID + 1]] 0000   [[#%.16x,STATIC:]] '_static_var'
----------------
int3 wrote:
> nit: can we keep the columns aligned? maybe call this `REL-DOT`
don't forget about this :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112291