[PATCH] D138334: Improve llvm-symbolizer search logic for symlink

[jeffrey tan via Phabricator via llvm-commits]

yinghuitan added inline comments.


================
Comment at: llvm/lib/Support/Signals.cpp:165
+    // parent directory as well.
+    if (llvm::sys::fs::is_symlink_file(Argv0)) {
+      llvm::SmallString<128> Resolved;
----------------
yinghuitan wrote:
> aganea wrote:
> > I feel adding the `paths` variable is adding complexity for future readers. Also, do we really need `is_symlink_file()`? Can we just call `real_path()`?
> > What about just this: (and keep the comment)
> > ```
> >    if (!LLVMSymbolizerPathOrErr) {
> >       llvm::SmallString<128> Resolved;
> >       if (!llvm::sys::fs::real_path(Argv0, Resolved)) {
> >         Parent = llvm::sys::path::parent_path(Resolved);
> >         if (!Parent.empty())
> >           LLVMSymbolizerPathOrErr = sys::findProgramByName("llvm-symbolizer", Parent);
> >       }
> >    }
> > ```
> > 
> `is_symlink_file()` underlying uses `stat()` API to check symlink while `real_path` I assume is more expensive. Also, semantic wise, I do not think they equal --  if `Argv0` is not a symlink, we do not want to retry `sys::findProgramByName` but `real_path` can't tell us about this fact. 
> 
> I can refactor the code to retry `sys::findProgramByName` if the first try failed instead of relying on the paths ordering though. 
Sorry, if this is not clear. What I mean is, for non-symlink path, `llvm::sys::fs::real_path()` wouldn't fail, so we will unnecessarily redo `sys::findProgramByName` which we shouldn't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138334