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

[jeffrey tan via Phabricator via llvm-commits]

yinghuitan added a comment.

@dblaikie, I did not find any existing test for it. Do you have any suggestion/example how to test this? We need to introduce a crash using this API and get the output and check the symbolication.



================
Comment at: llvm/lib/Support/Signals.cpp:165
+    // parent directory as well.
+    if (llvm::sys::fs::is_symlink_file(Argv0)) {
+      llvm::SmallString<128> Resolved;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138334