Page MenuHomePhabricator

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

Authored by friss on Sep 23 2014, 7:06 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

friss updated this revision to Diff 13992.Sep 23 2014, 7:06 AM
friss retitled this revision from to [dwarfdump] Print the name for referenced specification of abstract_origin DIEs..
friss added reviewers: dblaikie, samsonov, echristo, aprantl.
friss added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Sep 23 2014, 8:47 AM

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?

test/DebugInfo/inline-scopes.ll
27 ↗(On Diff #13992)

Why so many changes here? I assume this test would've passed unmodified even with your improvements (but I guess that's true of most of these test cases?)

friss added a comment.Sep 23 2014, 9:12 AM
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. 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 think only 1 or 2 tests needed real updates to pass. All the others I modified just add coverage for the introduced feature. 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).

test/DebugInfo/inline-scopes.ll
27 ↗(On Diff #13992)

I just added checks that we are FileChecking the right inlined_subroutine tree. yes, this test would have passed without modifications.

samsonov edited edge metadata.Sep 23 2014, 11:21 AM

Overall, I think that printing the name of referenced function makes sense and improves usability. I don't have strong opinion about the test cases (we should of course modify some of them to test the new behavior).

lib/DebugInfo/DWARFDebugInfoEntry.cpp
129 ↗(On Diff #13992)

instead of manually checking for DW_FORM_ref_sig8, can you check it like

formValue.getAsReference(u).hasValue()

?

friss added inline comments.Sep 23 2014, 11:54 AM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
129 ↗(On Diff #13992)

I'm not exactly sure what you're suggesting, but the issue is not that there might be no value, but that the value returned for DW_FORM_ref_sig8 attributes is non-sensical in this context (For ref_sig8 attributes, the returned value is a hash of the referenced DIE tree).
I've in fact been through a few iterations about this, because I don't find this check very elegant. In one of these, I created a new FormClass - FC_Signature - and thus I had the sig8 attributes return None for getAsReference(). This made the code above a bit nicer, but I didn't like that something that's explicitly named a reference (*ref*_sig8) would not be in the FC_Reference FormClass.

samsonov added inline comments.Sep 24 2014, 6:58 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
129 ↗(On Diff #13992)

Yes, supporting DW_FORM_ref_sig8 is tricky, and we have a FIXME in getAsReference(). Can valid subprogram DIE actually have DW_FORM_ref_sig8 in its DW_AT_specification/DW_AT_abstract_origin? I thought that ref_sig8 is now used only for type units.

It makes sense to have DW_FORM_ref_sig8 be FC_Reference to be consistent with what's written in DWARF standard. But we can improve out getAsReference() method - instead of just returning uint64_t it may return a pair: <Unit containing a referenced DIE, Offset of referenced DIE in a corresponding section>.

Anyway, all this is about future improvement. I think the patch is fine to go in as is, once you agree with David about the amount of changes to the test cases.

samsonov accepted this revision.Sep 24 2014, 6:58 PM
samsonov edited edge metadata.
This revision is now accepted and ready to land.Sep 24 2014, 6:58 PM
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).

friss added a comment.Sep 26 2014, 7:49 AM
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?

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.

friss updated this revision to Diff 14414.Oct 3 2014, 3:37 PM
friss edited edge metadata.

Modify all tests matching the target of DW_AT_abstract_origin or
DW_AT_specification attributes to use the introduced name resolving feature
of llvm-dwarfdump. The offset checking was left in 3 tests that are equally
interested in the layout of the Dwarf and in the correctness of the references.

A followup patch will generalize the feature to also resolve variable refs. The
current implementation only resolves subroutines.

dblaikie accepted this revision.Oct 3 2014, 3:45 PM
dblaikie edited edge metadata.

Looks good - just some optional bits/questions/ideas.

test/DebugInfo/Inputs/gmlt.ll
46 ↗(On Diff #14414)

Not sure - might even be worth dropping these abstract subprograms entirely when they're not checking anything interesting (just the name is being checked by the abstract_definition check you modified below). *shrug* dunno.

test/DebugInfo/X86/inline-member-function.ll
24 ↗(On Diff #14414)

Could write this as {{NULL|TAG}} (& update the other one nearby to do that for consistency). If you like.

test/DebugInfo/X86/inline-seldag-test.ll
14 ↗(On Diff #14414)

I see you dropped the abs def checking in this case - why this case & not others?

test/DebugInfo/inline-scopes.ll
29 ↗(On Diff #13992)

{{NULL|DW_TAG}} ?

friss added inline comments.Oct 3 2014, 3:58 PM
test/DebugInfo/Inputs/gmlt.ll
46 ↗(On Diff #14414)

Unfortunately, in some test cases they have to stay because they 'consume' a part of the file. If I had removed this one, then the next TAG_subprogram would match the abstract DIE and the test would fail. This is one of the biggest shortcomings of testing the Dwarf contents with FileCheck IMHO.

test/DebugInfo/X86/inline-member-function.ll
24 ↗(On Diff #14414)

Will do

test/DebugInfo/X86/inline-seldag-test.ll
14 ↗(On Diff #14414)

Because in this case, noone else tries to match a TAG_subprogram, the test bellow is for an inline_subroutine.

friss closed this revision.Oct 5 2014, 8:46 PM
friss updated this revision to Diff 14440.

Closed by commit rL219099 (authored by @friss).