[PATCH] [dwarfdump] Print the name for referenced specification of abstract_origin DIEs.

David Blaikie dblaikie at gmail.com
Thu Sep 25 11:01:44 PDT 2014


>>! In D5466#5, @friss wrote:
>>>! In D5466#4, @dblaikie wrote:
>> Looks fairly reasonable - not sure whether the test cases (if they pass without modifications) need updating. I'm undecided about if/how this feature will improve testing (in the existing cases, as you've kept, they really end up testing that the offsets actually refer to the desired DIE - I think that's valuable, rather than just testing the name (& at that point I'm not sure how much value we gain by testing the name as well)). What are your thoughts?
> 
> I think that the feature is very valuable as a llvm-dwarfdump user. 

Agreed, no contest.

> In a real dump, being able to identify directly inlined subroutines is really neat. For testing, I'm quite sure some tests can benefit from it. In the one you point out in the review for example, I test that the FileCheck patterns are actually checking the right function without having to match the offsets. There is little room for doubt in the test, but I think it is a nice and easy addition, if only as some sort of self-documentation of the test. Testing both the name and the offset - like it's done in many of the updated tests - doesn't bring any value except for testing the dumping feature in itself.

I'm not entirely sure here - but I don't /disagree/ as such. If that's the case, then could we remove the extra offset checking from those test cases?

> I think only 1 or 2 tests needed real updates to pass. All the others I modified just add coverage for the introduced feature. 

This kind of incidental coverage is somewhat problematic. If someone comes back tomorrow and says "oh, I can make these tests simpler by dropping the offset checking and just check the names - it still tests what it was intended to test that way" and we lose coverage on this feature. I think it might be best to head that off, simplify the tests as we think they should be written (probably dropping the offset checking entirely - as we do for checking strings, for example) and add explicit test coverage to the feature in a separate file with a clear name, comments, etc.

> I looked for tests that handle abstract_origins and specifications and just choose some of them (being sure to include exotic stuff like cross-cu references).

http://reviews.llvm.org/D5466






More information about the llvm-commits mailing list