[DWARF] check whether the DIE is valid before querying for information

[paulsemel]

Summary

This prevents for having an assert instead of just returning an empty DIEsForAddress object.

[dblaikie]

Could you include a test case?

[paulsemel]

Add test case.

[dblaikie]

llvm/test/DebugInfo/dwarfdump-bad-lookup-address.test

1 ↗

(On Diff #193367)

Testing that the program doesn't crash is a fairly broad test (what does it do instead? It could print all sorts of odd/weird/incorrect output while still passing this test) - could you test for the specific output that was hidden behind this crash previously?

[paulsemel]

Add correct output of llvm-dwarfdump

[dblaikie]

llvm/test/DebugInfo/dwarfdump-bad-lookup-address.test

1 ↗

(On Diff #193367)

Hmm, could you help me understand under what conditions the crash occurs? And why printing the compile_unit is the correct behavior here?

In other cases I've just tested, passing lookup a value it can't find - had a simple file with two functions ([0-6) and [16-22)). Querying for zero actually seems (probably unrelatedly) buggy - it prints all 3 DIEs (the CU and both subprograms).

0: prints all 3 DIEs
[1-5]: prints the first subprogram DIE (& the CU parent)
[6-15]: crashes (probably this bug)
[16-21]: prints the second subprogram DIE (& the CU parent)
[22-...: prints the file format header and nothing else.

Hmm, I suppose, then, it makes sense to print the CU - because the address is within the range of the CU but not within the range of anything inside of it.

Maybe add a "CHECK-NOT: DW_TAG" at the end of the checks?
(& really it might not be necessary to check all the attributes - we know all that dumping works)
& a comment in the test file describing the purpose of the test value?

(& not sure - but perhaps this test would be easier to understand if it had its own test file, rather than using a larger/more complicated one from another test)

[paulsemel]

Add comment describing the bug.

[dblaikie]

Looks good - thanks!

[paulsemel]

llvm/test/DebugInfo/dwarfdump-bad-lookup-address.test

1 ↗

(On Diff #193367)

I think you're right. As the address only belongs to the compile unit, this is all we want to be printed here.

Do you really think that we need a separate file. The bug pretty much describes itself I think.

[dblaikie]

llvm/test/DebugInfo/dwarfdump-bad-lookup-address.test

1 ↗

(On Diff #193367)

The comments are probably sufficient - but if you liked, I wouldn't object to a separate test file *shrug* all good :)

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

DWARFContext::DIEsForAddress DWARFContext::getDIEsForAddress(uint64_t Address) {
   Result.FunctionDIE = CU->getSubroutineForAddress(Address);
 
   std::vector<DWARFDie> Worklist;
   Worklist.push_back(Result.FunctionDIE);
   while (!Worklist.empty()) {
     DWARFDie DIE = Worklist.back();
     Worklist.pop_back();
 
+    if (!DIE.isValid())
+      continue;
+
     if (DIE.getTag() == DW_TAG_lexical_block &&
         DIE.addressRangeContainsAddress(Address)) {
       Result.BlockDIE = DIE;
       break;
     }
 
     for (auto Child : DIE)
       Worklist.push_back(Child);

llvm/trunk/test/DebugInfo/dwarfdump-bad-lookup-address.test

+#RUN: llvm-dwarfdump -lookup 1161 %p/Inputs/dwarfdump-test.macho-i386.o | FileCheck %s
+
+// When we pass an address to getDIEsFromAddress that doesn't belong to any DIE
+// (only to a CU), this function is triggering an assert, which is not the
+// intended behavior (as the address still belongs to a CU).
+
+#CHECK: DW_TAG_compile_unit
+#CHECK-NEXT:   DW_AT_producer{{.*}}"clang version 3.6.2 (tags/RELEASE_362/final)"
+#CHECK-NEXT:   DW_AT_language{{.*}}DW_LANG_C_plus_plus
+#CHECK-NEXT:   DW_AT_name{{.*}}"dwarfdump-test.cc"
+#CHECK-NOT:    DW_TAG