This is an archive of the discontinued LLVM Phabricator instance.

Handle mixed-OS paths in DWARF reader
ClosedPublic

Authored by eugene on Mar 8 2018, 11:58 PM.

Details

Summary

Make sure that DWARF line information generated by Windows can be properly read by Posix OS and vice versa.

Diff Detail

Event Timeline

eugene created this revision.Mar 8 2018, 11:58 PM
eugene updated this revision to Diff 137703.Mar 9 2018, 1:31 AM

clang-format

This will also need a testcase.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
959

Shouldn't this function be provided in libSupport? For example, by adding a new enumerator sys::path::Style::any ?

eugene updated this revision to Diff 137904.Mar 10 2018, 2:52 AM

Added a test

Is there some kind of field we can write inside the DWARF that can tell us what path syntax to assume? I'm a little weary about trying to guess. It seems like there could be lots of edge cases where we incorrectly determine what syntax a path is using.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
959

It seems pretty special purpose. If we were going to put something in support I would rather have it be something like sys::path::guess_path_syntax(const Twine &Path)

Is there some kind of field we can write inside the DWARF that can tell us what path syntax to assume?

As far as I know DWARF doesn't have a field like this.

I'm a little weary about trying to guess. It seems like there could be lots of edge cases where we incorrectly determine what syntax a path is using.

I share your worry for the general practice, but in this particular case I think it's ok.
If path is absolute on any OS (starts with / or X:) the code will treat it like it is absolute, and just won't try to concatenate more things together.

On Windows Android NDK produces binaries where different CUs have different path styles. Those binaries need to be symbolized on Linux and Mac OS, hence this change removing assumption that path style is always native.

I think that adding a path-style field to dwarf (it would probably have to be a vendor extension) is an excellent idea. However, that won't help us with parsing the existing debug info corpus. This isn't going to be 100% correct (e.g. c:\foo.cpp is a valid relative path on unix), but I hope nobody actually uses file names where that matters. If someone like that turns up, then we'd probably have to add a some flag about how the user wants the paths to be treated, as there is no way we can figure out the correct behavior automatically.

aprantl accepted this revision.Mar 12 2018, 9:09 AM

Oh well, I think you convinced me.

This revision is now accepted and ready to land.Mar 12 2018, 9:09 AM
This revision was automatically updated to reflect the committed changes.