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

Frederic Riss friss at apple.com
Fri Sep 26 07:49:59 PDT 2014


>>! In D5466#11, @dblaikie wrote:
>> 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 we could. There is one small pitfall that I can see though: static symbols (or symbols in anonymous namespaces) could end up with the same name even though they represent different symbols. I don't think this would be an issue for any of the tests.

>> 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'm a bit confused. If someone simplifies the test by removing the offsets, then we still cover the introduced feature. Or maybe by 'the feature' you mean the actual printing of the offsets? I agree that simplifying the tests is probably better, but are you actually suggesting that we should then add a test that explicitly checks the offsets?

http://reviews.llvm.org/D5466






More information about the llvm-commits mailing list