Page MenuHomePhabricator

[dwarfdump] Prettyprint DW_AT_APPLE_property_attribute bitfield values.
ClosedPublic

Authored by friss on Oct 8 2014, 5:14 PM.

Details

Summary

This change depends on the ApplePropertyString helper that I sent spearately.
Not sure how you want this tested: as a tool test by adding a binary to dump, or as an llvm test starting from an IR file?

Diff Detail

Repository
rL LLVM

Event Timeline

friss updated this revision to Diff 14616.Oct 8 2014, 5:14 PM
friss retitled this revision from to [dwarfdump] Prettyprint DW_AT_APPLE_property_attribute bitfield values..
friss added reviewers: dblaikie, samsonov.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Oct 8 2014, 5:17 PM

Looks reasonable (apart from testing) - depends how you want to do it. I assume there are some existing tests that test properties that could be updated to use this feature? (does this not regress any existing tests?)

If there aren't any such tests, I imagine we want some - and they could just test this incidentally.

(I'm usually on the fence about writing tool tests separately from unit tests... *shrug* sometimes it's nice, sometimes it seems like overkill)

friss added a comment.Oct 8 2014, 5:22 PM
In D5689#4, @dblaikie wrote:

Looks reasonable (apart from testing) - depends how you want to do it. I assume there are some existing tests that test properties that could be updated to use this feature? (does this not regress any existing tests?)

If there aren't any such tests, I imagine we want some - and they could just test this incidentally.

(I'm usually on the fence about writing tool tests separately from unit tests... *shrug* sometimes it's nice, sometimes it seems like overkill)

There aren't any. I discovered the discrepancy between the enum values yesterday by trying to produce some tests and the results didn't actually make sense. I can add an llvm test, sure, but my ObjC foo being what it is, I fear that the code won't make any sense :-) Thus it won't test much except the actual dumping.

Probably sensible enough then to just make it a dumping test - I think
that's generally 'better'/purist when adding dumper features, I just
understand it's sometimes a bit more fuss. So I'll never complain about you
doing it that way (checking a small sample source, a binary, etc - you can
see the other pure dwarfdump tests).

& since you guys are planning to make this more of a production tool, I
think it's a good idea to have more tests for dwarfdump itself - so that we
don't accidentally make holes in the dwarfdump test coverage when we
refactor tests.

samsonov edited edge metadata.Oct 8 2014, 5:35 PM

Yep, looks good once we test this.

lib/DebugInfo/DWARFDebugInfoEntry.cpp
87 ↗(On Diff #14616)

This would output an extra trailing comma

friss added inline comments.Oct 8 2014, 5:57 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
87 ↗(On Diff #14616)

Nope because the condition just above exits if we've handled the last bit. (The actual loop exit test is in the middle of the loop. I find this idiom less clumsy than having a bool lying around telling if you are in the first iteration or not, but I can understand opinions might differ on this point).

samsonov added inline comments.Oct 8 2014, 6:17 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
87 ↗(On Diff #14616)

Oh, sorry, I missed that =/ The code is fine as is, I have no strong preference.

friss updated this revision to Diff 14680.Oct 9 2014, 3:10 PM
friss edited edge metadata.

Added a dwarfdump tool test for ObjC functionality (for now properties dumping).

dblaikie added inline comments.Oct 9 2014, 3:19 PM
test/DebugInfo/dwarfdump-objc.test
21 ↗(On Diff #14680)

(0x0c ( DW_APPLE_PROPERTY_foo, bar ) )

seems inconsistent use of space - there's no leading space before the 0x0c, but there's a leading space before DW (& then trailing spaces for both ')'.

I'd probably be in favor of no leading or trailing spaces, but I'm not sure what this looks like compared to various other parts of the dump output.

friss added inline comments.Oct 9 2014, 3:41 PM
test/DebugInfo/dwarfdump-objc.test
21 ↗(On Diff #14680)

Agreed, it doesn't look nice. I just copied the formatting from the native dwarfdump, but I like it better with leading and trailing spaces (there isn't any prior example of that kind of dump input I think, so just pick what seems nicer). I'll resubmit shortly.

friss updated this revision to Diff 14684.Oct 9 2014, 3:42 PM

Removed some spaces from the dump format.

dblaikie accepted this revision.Oct 9 2014, 5:19 PM
dblaikie edited edge metadata.
This revision is now accepted and ready to land.Oct 9 2014, 5:19 PM

Looks good to me - please submit.

(it seems most of our existing formatting doesn't put extra spaces - though we're not entirely consistent (strp has a leading space, exprloc (& maybe other FORM_data) has a trailing space (probably just because someone didn't handle the end case), etc)

friss closed this revision.Oct 10 2014, 9:01 AM
friss updated this revision to Diff 14735.

Closed by commit rL219507 (authored by @friss).