Page MenuHomePhabricator

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

Authored by paulsemel on Apr 2 2019, 12:55 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Apr 2 2019, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 12:55 PM

Could you include a test case?

paulsemel updated this revision to Diff 193367.Apr 2 2019, 2:45 PM

Add test case.

dblaikie added inline comments.Apr 2 2019, 3:52 PM
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 updated this revision to Diff 193389.Apr 2 2019, 4:26 PM
paulsemel marked an inline comment as done.

Add correct output of llvm-dwarfdump

dblaikie added inline comments.Apr 2 2019, 4:54 PM
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)

ormris removed a subscriber: ormris.Apr 2 2019, 5:14 PM
paulsemel updated this revision to Diff 193398.Apr 2 2019, 5:20 PM
paulsemel marked an inline comment as not done.

Add comment describing the bug.

dblaikie accepted this revision.Apr 2 2019, 5:22 PM

Looks good - thanks!

This revision is now accepted and ready to land.Apr 2 2019, 5:22 PM
paulsemel marked an inline comment as done.Apr 2 2019, 5:22 PM
paulsemel added inline comments.
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 added inline comments.Apr 2 2019, 5:25 PM
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 :)

This revision was automatically updated to reflect the committed changes.