[PATCH] D57916: [DebugInfo] Fix /usr/lib/debug llvm-symbolizer lookup with relative paths

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:146-154
+static cl::opt<std::string> ClFallbackDebugPath(
+    "fallback-debug-path", cl::init(""),
+    cl::desc("Fallback path for debug binaries. If empty, defaults to "
+#if defined(__NetBSD__)
+             "/usr/libdata/debug"
+#else
+             "/usr/lib/debug"
----------------
dblaikie wrote:
> rupprecht wrote:
> > dblaikie wrote:
> > > If these default values are going to be encoded here - perhaps both versions (for diagnostic printing and for the implementation) should be here, using the cl::init value to propagate the default from here & remove it from Symbolize.cpp?
> > > 
> > > (alternatively, perhaps it's not important enough to specify what the default is in the diagnostic? Dunno)
> > > 
> > > Just a thought.
> > There's a lot more paths through findDebugBinary, so removing the default from there would mean adding it to all call sites (there might be ~10 or so based on a quick check)
> > 
> > Including the default fallback in --help doesn't seem too important, so I'll just remove it.
> Oh, I didn't mean having to pass it around separately - I meant using the cl::init value, something like this I would think:
> 
>   #if defined(__NetBSD__)
>   #define DEFAULT_DEBUG_PATH "/usr/libdata/debug"
>   #else
>   #define DEFAULT_DEBUG_PATH "/usr/lib/debug"
>   #endif
>   static cl::opt<std::string> ClFallbackDebugPath(
>       "fallback-debug-path", cl::init(DEFAULT_DEBUG_PATH),
>       cl::desc("Fallback path for debug binaries. If empty, defaults to "
>             DEFAULT_DEBUG_PATH));
> 
> At least I think that'd work, probably?
> 
> That way the parameter support and defaults are all here - the code later on uses whatever value is passed to it & doesn't have to worry about it being empty & falling back to values hardcoded over there, etc. Does that make sense?
> 
> (but no worries either way)
Yeah, that should work. But the flip side is if some new platform has a different default, we'd have to update both llvm-symbolizer and Symbolizer.cpp to handle it; or more likely, we'd only update Symbolizer.cpp and then wonder why we weren't getting the new default path w/ llvm-symbolizer.

Probably the best thing would be expose the default value in Symbolizer.h, but that's just more plumbing than I think it's worth, especially since this probably won't be used outside this unit test :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57916