Page MenuHomePhabricator

Move ParsedAttrInfos into a registry and point to one in ParsedAttr
ClosedPublic

Authored by john.brawn on Mar 24 2017, 9:25 AM.

Details

Summary

This allows plugins to define an attribute and its associated ParsedAttrInfo, though the attribute won't be able to do anything. Also we end up removing AttrKinds.inc as searching through the registry replaces it.

Diff Detail

Event Timeline

john.brawn created this revision.Mar 24 2017, 9:25 AM

@john.brawn Are you still working on that? We would need this change at Mozilla to improve the way we use attributes.
Thanks

john.brawn retitled this revision from Move ParsedAttrInfos into a registry and point to one in AttributeList to Move ParsedAttrInfos into a registry and point to one in ParsedAttr.
john.brawn set the repository for this revision to rG LLVM Github Monorepo.
john.brawn added a subscriber: cfe-commits.

Resurrecting this old patch, with a bunch of changes due to the changes around AttributeCommonInfo. This is the second of four patches to make it possible for clang plugins to define attributes.

Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2020, 6:09 AM

Updated to match latest changes to D31337.

carwil added a subscriber: carwil.Jan 27 2020, 2:13 AM
aaron.ballman added inline comments.Feb 2 2020, 9:53 AM
clang/include/clang/Sema/ParsedAttr.h
45

Please add a full stop to the end of all the comments (here and elsewhere).

63

spelt -> spelled

609

I think this should return a const ParsedAttrInfo& so that callers don't try to mutate it.

clang/lib/Basic/Attributes.cpp
100–102

Range-based for loop? Also, it and ie don't meet the usual naming conventions.

One thing I'm not keen on with this is the performance hit. We spent a decent amount of effort making this lookup fast and it's now a linear search through all of the attributes and requires memory allocations and deallocations to perform the search.

112

This seems surprising because it makes it harder to determine whether you have a valid result from this function or not. I think this should return a null unique_ptr.

116

Same concerns in this function as above.

john.brawn marked an inline comment as done.Feb 4 2020, 10:06 AM
john.brawn added inline comments.
clang/lib/Basic/Attributes.cpp
100–102

Yes, I've done some experiments and in the pathological case (lots of trivial functions with the xray_log_args attribute) the slowdown is noticeable. I've done some tinkering and I think the best way to resolve this is to first use a generated function (i.e. something like the current getAttrKind) to look up the attributes that are compiled into clang, then if that fails look in the registry to find a match.

aaron.ballman added inline comments.Feb 23 2020, 7:22 AM
clang/lib/Basic/Attributes.cpp
100–102

I think that approach makes sense and is something we should probably do up front. It doesn't seem like llvm::Registry has a way for us to mark where the plugin attributes start to allow for quickly searching for plugin attributes in the fallback case.

I don't see a particularly good way around the memory allocations, however. Ideally, this could be something that's arena allocated on the ASTContext rather than using unique_ptr, but I think that would be tricky here because of library layering. After switching the lookup functionality, I'd be curious to know if there is a noticeable performance degredation still, or if that falls out into the noise.

This has been rewritten so that the registry is only used for attributes added by plugins, and everything else continues as before. This also removes the strange dependency I'd previously introduced, where Attributes.cpp (in libclangBasic.so) declared and used the registry but ParsedAttr.cpp (in libclangSema.so) populated it - now the registry is declared and used solely within ParsedAttr.cpp.

aaron.ballman accepted this revision.Feb 27 2020, 7:14 AM

I like this approach much better, thank you for working on it! LGTM

clang/include/clang/Basic/AttributeCommonInfo.h
140

This should probably be getNormalizedFullName() -- the full by itself implies we don't normalize, to me.

clang/include/clang/Sema/ParsedAttr.h
66

NormalizedFullName?

This revision is now accepted and ready to land.Feb 27 2020, 7:14 AM
This revision was automatically updated to reflect the committed changes.