This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Don't resolve DIE reference to NULL DIE.
ClosedPublic

Authored by JDevlieghere on Sep 20 2017, 6:07 AM.

Details

Summary

This patch prevents dsymutil from resolving a reference to a NULL DIE
when a bogus reference happens to be coincidentally referencing a NULL
DIE. Now this is detected as an invalid reference and a warning is
printed.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=33873

Diff Detail

Event Timeline

JDevlieghere created this revision.Sep 20 2017, 6:07 AM
friss added inline comments.Sep 20 2017, 8:27 AM
test/tools/dsymutil/X86/null-die.s
10 ↗(On Diff #115993)

Do you need the final binary file? My guess is that you only need the .o file and a debug map which can be YAML. You can use 'llvm-dsymutil -dump-debug-map' on null_die to extract it.

tools/dsymutil/DwarfLinker.cpp
1498–1503

I would add a comment here. There is no valid reason for the DIE to be NULL. Something along the lines of "In a file with broken references, an attribute might point to a NULL DIE".

dblaikie added inline comments.
test/tools/dsymutil/X86/null-die.s
4–10 ↗(On Diff #115993)

This seems like a rather large test case for this small issue?

  • Address review comments from Fred and David.
  • Convert test to YAML with instruction on how to reproduce the binary.
JDevlieghere marked 3 inline comments as done.Sep 20 2017, 9:48 AM
  • Remove relative path in test.
dblaikie added inline comments.Sep 20 2017, 9:58 AM
test/tools/dsymutil/X86/null-die.test
11–20

This DWARF could be a bit simpler by skipping argc and argv and having foo be void() rather than int(int), have foo call some other function & compile without optimizations:

void f1() {}
__attribute__((always_inline)) void f2() {
  f1();
}
int main() {
  f2();
}

(marginally simpler if 'int main' were just 'void f3' - though then I guess you'd need a stub object (which wouldn't need debug info anyway) to link in main, not sure, which might be more hassle than necessary - if you did that you could also put f1 out of line in that object file so there would be no DWARF for f1 either, again simplifying the test case a bit)

Also potentially simpler if this is in C++ and f2 is marked 'inline' as well, that way there's no out of line definition of f2.

JDevlieghere set the repository for this revision to rL LLVM.

Thanks David, I appreciate your feedback!

JDevlieghere marked an inline comment as done.Sep 20 2017, 10:24 AM
friss accepted this revision.Sep 20 2017, 10:37 AM

I think the test can go in the generic test directory. If you're not generating any output, it shouldn't need the X86 backend.

Otherwise, LGTM

This revision is now accepted and ready to land.Sep 20 2017, 10:37 AM
This revision was automatically updated to reflect the committed changes.