[llvm-dwp] Join dwo paths correctly when DWOPath is absolute

[nagisa]

Summary

When the DWOPath is absolute, we want to use DWOPath as is, without prepending any other
components to the path. The sys::path::append does not join, but rather unconditionally appends
the paths, so something like sys::path::append("/tmp", "/tmp/banana") will result in
/tmp/tmp/banana rather than the desired /tmp/banana.

This then causes llvm-dwp to fail in a following situation:

$ clang -gsplit-dwarf /tmp/banana/test.c -c -o /tmp/outdir/foo.o
$ clang outdir/foo.o -o outdir/hm
$ llvm-dwarfdump outdir/hm | grep -C2 foo.dwo
                  DW_AT_comp_dir    ("/tmp")
                  DW_AT_GNU_pubnames  (true)
                  DW_AT_GNU_dwo_name    ("/tmp/outdir/foo.dwo")
                                DW_AT_GNU_dwo_id    (0xde4d396f3bf0e257)
                  DW_AT_low_pc  (0x0000000000401100)
$ strace -o trace llvm-dwp -e outdir/hm -o outdir/hm.dwp
error: No such file or directory
$ cat trace | grep foo.dwo
openat(AT_FDCWD, "/tmp/tmp/outdir/foo.dwo", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

[nagisa]

Note: I do not have a commit bit, if approving this, please also commit.

[dblaikie]

Please add/include test coverage.

llvm/tools/llvm-dwp/llvm-dwp.cpp

529

Generally prefer = init syntax over {} or () (this avoids any invocation of explicit conversions, for instance - making the code a bit simpler/easier to understand)

[nagisa]

Add a test for llvm-dwp operation with absolute paths

[nagisa]

diff with lint

[nagisa]

Please add/include test coverage.

Added a test and verified it reproduces the issue without this change and passes now. Let me know if this approach is not what you were thinking of.

llvm/tools/llvm-dwp/llvm-dwp.cpp

529

The = fails to build, because the conversion is necessary (DWOName is a std::string). Is there a better way to get this conversion happen?

[nagisa]

Remove some fluff from the test llvm-ir

[dblaikie]

llvm/test/tools/llvm-dwp/X86/absolute_paths.test

4 ↗

(On Diff #323836)

Should this have some CHECKs for certain behavior (dumping the resulting dwp and checking things, for instance)

"does something other than crashing/failing" is a pretty low bar for a test, doesn't especially check that this behaves in a way we would want it to.

llvm/tools/llvm-dwp/llvm-dwp.cpp

529

Ah, no, that's fine then. I'd /probably/ lean towards using () rather than {} then, because there's been no wide-scale stylistic tendency towards {} for ctors in LLVM, but not a huge deal.

[nagisa]

Add some filecheck to the test and change the constructor style

[nagisa]

llvm/test/tools/llvm-dwp/X86/absolute_paths.test

4 ↗

(On Diff #323836)

I added some CHECK lines that should at least verify that the llvm-dwp picks up the right dwo file (by the virtue of both the files having (mostly?) the same contents).

I had a pretty hard time coming up with something more meaningful than that.

[dblaikie]

Sounds good, thanks for walking me through it.

(as a heads up: llvm-dwp isn't great (I wrote it years ago to mitigate some risk with binutils dwp memory usage which ended up being resolved otherwise) - specifically I don't think it's ever been used in any production use case that I know of, has I believe significantly worse runtime/memory usage than binutils/gold dwp and while neither supports DWARFv5, I do have some patches I need to upstream to add DWARFv5 support for binutils/gold dwp

At some point llvm-dwp probably will be/needs to be rewritten, probably using some of lld's infrastructure (in a similar way to binutils/gold dwp uses some gold infrastructure) to reduce overheads.

But small patches with tests like this aren't too bad and the tests will ensure we preserve the functionality during any such rewrite in the future)

llvm/test/tools/llvm-dwp/X86/absolute_paths.test

8–15 ↗

(On Diff #324066)

Maybe only dump/test the debug_info section ( llvm-dwarfdump %t/test.dwp should do that, only dumping the debug_info section by default) and check DW_AT_subprogram has a DW_AT_name of "banana" - would check this specific dwo file was included in the dwp (& indirectly exercises a bunch of other functionality, like index usage, strings, abbreviations, etc).

[nagisa]

Check the formatted output from dwarfdump instead

[nagisa]

revert the test to its previous form

[nagisa]

Just so that it doesn't get lost between hidden comments: I've no commit bit, please commit this for me if this is good to go.

llvm/test/tools/llvm-dwp/X86/absolute_paths.test

8–15 ↗

(On Diff #324066)

Looks like doing so doesn't work out in this particular test. On windows the dwarfdump output gets escaped in a way and -DPATH that we pass in is not escaped (the same way). cf. this. I'm not seeing a good way to match the string so I'm going to keep the test as it was originally written.

llvm/tools/llvm-dwp/llvm-dwp.cpp

for (const auto &CU : DWARFCtx->compile_units()) {
     const DWARFDie &Die = CU->getUnitDIE();
     std::string DWOName = dwarf::toString(
         Die.find({dwarf::DW_AT_dwo_name, dwarf::DW_AT_GNU_dwo_name}), "");
     if (DWOName.empty())
       continue;
     std::string DWOCompDir =
         dwarf::toString(Die.find(dwarf::DW_AT_comp_dir), "");
     if (!DWOCompDir.empty()) {
-      SmallString<16> DWOPath;
-      sys::path::append(DWOPath, DWOCompDir, DWOName);
+      SmallString<16> DWOPath{std::move(DWOName)};
+      sys::fs::make_absolute(DWOCompDir, DWOPath);
       DWOPaths.emplace_back(DWOPath.data(), DWOPath.size());
     } else {
       DWOPaths.push_back(std::move(DWOName));
     }
   }
   return std::move(DWOPaths);
 }