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

[David Blaikie]

>>! In D5466#12, @friss wrote:
>>>! 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.

Right - if we have a test for which that's an issue, use the extra offset testing (& probably drop the names) & include a comment explaining why that's interesting. But as you say, I doubt we have any tests for which that's an issue.

>>> 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?

Fair point, that the tests will still cover your new functionality - though I'm still a little on the fence about testing dumper functionality in LLVM feature tests rather than in separate dumper tests. It's not a big deal though - I've gone back & forth on it myself (in some cases checking in binaries and adding specific llvm-dwarfdump tests, then adding separate tests for the LLVM functionality I wanted to test, and in some cases cutting corners and just introducing the dumper functionality and testing LLVM immediately without a dumper test)

Long story short: If you want to update test cases, please update them to their "ideal" form, how you think they should be written if the feature had been available - rather than a little of the old and a little of the new, that would just be confusing to future readers ("wait, why is it testing this thing and this other thing? Am I missing some details that make that important/relevant?"). If you want to be extra diligent, add an explicit dumper test for the functionality you're adding.

http://reviews.llvm.org/D5466