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

Tue Apr 2 17:27:55 PDT 2019

dblaikie 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
----------------
paulsemel wrote:
> 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.
The comments are probably sufficient - but if you liked, I wouldn't object to a separate test file *shrug* all good :)

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60147