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

[Paul Semel via Phabricator via llvm-commits]

paulsemel marked an inline comment as done.
paulsemel added inline comments.


================
Comment at: llvm/test/DebugInfo/dwarfdump-bad-lookup-address.test:1
+RUN: llvm-dwarfdump -lookup 1161 %p/Inputs/dwarfdump-test.macho-i386.o
----------------
dblaikie wrote:
> dblaikie wrote:
> > 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?
> 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)
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60147/new/

https://reviews.llvm.org/D60147