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

[Greg Clayton via Phabricator via llvm-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Support/Signals.cpp:141
+/// and return its parent path.
+static StringRef resolveSymlinkParent(StringRef File) {
+  StringRef ResolvedParent;
----------------
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.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138334