[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:141
+/// and return its parent path.
+static StringRef resolveSymlinkParent(StringRef File) {
+  StringRef ResolvedParent;
----------------
clayborg wrote:
> You will run into lifetime issues with the returned StringRef in ResolvedParent if we have a symlink as the data in the ResolvedParent will point to data in the "Resolved" variable. This variable contains the storage for the string in this scope, which will free its string when it destructs before returning from this function. Most of the time it will be ok because it is on the stack, but we can check this in. I would suggest inlining this code into the function below so that object lifetimes stay around long enough to get used. Or you can store "llvm::SmallString<128> Resolved;" in the printSymbolizedStackTrace() function as a local, and pass a reference to it into this function. That way the buffer will stay along long enough for the StringRef to not point to stale data.
Thanks! I hate falling into this kind of trap, I really should remember return `StringRef` is like returning a pointer...


================
Comment at: llvm/lib/Support/Signals.cpp:172-180
     StringRef Parent = llvm::sys::path::parent_path(Argv0);
+    std::vector<StringRef> paths;
+    if (!Parent.empty())
+      paths.push_back(Parent);
+    Parent = resolveSymlinkParent(Argv0);
     if (!Parent.empty())
+      paths.push_back(Parent);
----------------
clayborg wrote:
> We should probably try to get the llvm-symbolizer as we did before, and if this fails, then fall back to doing the symlink thing. You might have a directory with newly builds binaries in it, and just use a symlink to point to an older binary.
> 
> So I would vote to:
> 1 - check as we did before
> 2 - if there is an error, fallback to trying the symlink method.
> 
> We are currently relying on the parent being a symlink. What is if the binary itself is a symlink and not the parent? I would think we would want to resolve the path to the binary first, then call llvm::sys::path::parent_path(), and then look for llvm-symbolizer. The above code removes the binary from the path first, then assumes the parent will be a symlink.
@clayborg , I think you might have read the code incorrectly:
1. The code actually only deals with executable itself being symlink not its parent. 
2. The code only adds an extra search path. And `sys::findProgramByName` would use `paths` in priority order -- if it finds llvm-symbolizer in the first path it would try the second one (resolved symlink real path). Actually, there is indeed a bug due to refactoring -- I accidentally passed Parent instead "paths". I will fix that.

So it should do the logic what you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138334