Page MenuHomePhabricator

[DebugInfo] Fix /usr/lib/debug llvm-symbolizer lookup with relative paths
ClosedPublic

Authored by rupprecht on Feb 7 2019, 11:08 AM.

Details

Summary

rL189250 added a realpath call, and rL352916 because realpath breaks assumptions with some build systems. However, the /usr/lib/debug case has been clarified, falling back to /usr/lib/debug is currently broken if the obj passed in is a relative path. Adding a call to use absolute paths when falling back to /usr/lib/debug fixes that while still not making any realpath assumptions.

This also adds a --fallback-debug-path command line flag for testing (since we probably can't write to /usr/lib/debug from buildbot environments), but was also verified manually:

$ rm -f path/to/dwarfdump-test.elf-x86-64
$ strace llvm-symbolizer --obj=relative/path/to/dwarfdump-test.elf-x86-64.debuglink 0x40113f |& grep dwarfdump

Lookups went to relative/path/to/dwarfdump-test.elf-x86-64, relative/path/to/.debug/dwarfdump-test.elf-x86-64, and then finally /usr/lib/debug/absolute/path/to/dwarfdump-test.elf-x86-64.

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Feb 7 2019, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 11:08 AM

One option for testing would be to move DebugPath into a user-configurable value (something that could be passed on the command line), for testing purposes. Not sure if it's worth it, though.

rupprecht updated this revision to Diff 186092.Feb 8 2019, 6:17 PM
  • Add --fallback-debug-path flag for testing /usr/lib/debug fallback
rupprecht edited the summary of this revision. (Show Details)Feb 8 2019, 6:18 PM
dblaikie accepted this revision.Feb 9 2019, 11:31 AM
dblaikie added inline comments.
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
146–154 ↗(On Diff #186092)

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.

This revision is now accepted and ready to land.Feb 9 2019, 11:31 AM
rupprecht marked 2 inline comments as done.Feb 11 2019, 10:02 AM
rupprecht added inline comments.
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
146–154 ↗(On Diff #186092)

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.

This revision was automatically updated to reflect the committed changes.
rupprecht marked an inline comment as done.
dblaikie added inline comments.Feb 11 2019, 12:38 PM
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
146–154 ↗(On Diff #186092)

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)

rupprecht marked an inline comment as done.Feb 11 2019, 1:39 PM
rupprecht added inline comments.
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
146–154 ↗(On Diff #186092)

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 :)

dblaikie added inline comments.Feb 11 2019, 1:40 PM
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
146–154 ↗(On Diff #186092)

Oh, sorry, I get what you mean now - makes sense. Thanks!