This is an archive of the discontinued LLVM Phabricator instance.

Update dwarf::ApplePropertyAttributes enum to meaningful values.
ClosedPublic

Authored by friss on Oct 7 2014, 11:21 AM.

Details

Summary

We currently emit an DW_AT_APPLE_property_attribute with a value that is a
bitfield describing the various attributes applied to an ObjectiveC property.
While trying to add testing to one of my dwarfdump patches that would pretty
print that, I realized this information looks totally broken and has maybe
never been correct.

As with every DWARF info, we have some enum in Dwarf.h that describes this
attribute (enum ApplePropertyAttributes). It seems however that the attribute
value is set from another definition of these flags in Sema/DeclSpec.h (enum
ObjCPropertyAttributeKind). And these 2 enums aren't in sync.

This patch updates the Dwarf.h values to the ones we are (and have been for
a very long time) emitting. We change some publicly (and even documented
in SourceLevelDebugging.rst) values, but I doubt this could be an issue as
the information has been wrong for so long...

Diff Detail

Repository
rL LLVM

Event Timeline

friss updated this revision to Diff 14520.Oct 7 2014, 11:21 AM
friss retitled this revision from to Update dwarf::ApplePropertyAttributes enum to meaningful values..
friss added reviewers: echristo, dblaikie, aprantl.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Oct 7 2014, 11:59 AM

I'll leave this to Adrian to sign off on, since it's out of my depth in ObjC land, but I can confirm that this enum now matches the other one.

include/llvm/Support/Dwarf.h
784 ↗(On Diff #14520)

Crazy idea: Could we do this the other way around, and remove ObjCPropertyAttributeKind in favor of using this enum directly?

Probably not, but figured I'd mention it.

echristo accepted this revision.Oct 7 2014, 12:00 PM
echristo edited edge metadata.

Seems obvious. Thanks.

-eric

This revision is now accepted and ready to land.Oct 7 2014, 12:00 PM
friss added inline comments.Oct 7 2014, 12:57 PM
include/llvm/Support/Dwarf.h
784 ↗(On Diff #14520)

I thought of that but I think there would be (rightful) resistance to having Sema code depend on the Dwarf header. In a followup patch for the dwarfdump functionality, I'll try to add coverage for most if not all of these, that will at least prevent us from breaking the current values unknowingly.

friss closed this revision.Oct 8 2014, 8:09 AM
friss updated this revision to Diff 14578.

Closed by commit rL219311 (authored by @friss).