[APINotes] Add support for bounds safety annotations #9822
Conversation
This adds support for annotating function parameters with __counted_by, __sized_by, __counted_by_or_null, __sized_by_or_null, and __ended_by, using API notes. The main content of handlePtrCountedByEndedByAttr is extracted to applyPtrCountedByEndedByAttr and decoupled from ParsedAttr. The helper function ParseBoundsAttributeArgFromString is added to make it possible to parse count expressions from SemaAPINotes. The current implementation of __terminated_by/__null_terminated makes it harder to extract from the iterative type processing, but since it doesn't require any extra context to parse the attribute, it can be applied using the normal Type override instead. rdar://139830881
hnrklssn
requested review from
delcypher,
Xazax-hun,
DougGregor,
egorzhdan,
rapidsna,
patrykstefanski,
fahadnayyar,
danliew-apple and
apple-fcloutier
|
@swift-ci please smoke test |
|
Are these attributes supported in upstream Clang? If so, this PR should probably be upstream. |
|
Could you please add |
| BoundsSafety: | ||
| Kind: counted_by | ||
| Level: 1 | ||
| BoundedBy: "*len" |
|
It may worth adding some more tests:
|
|
| @@ -8941,6 +8941,88 @@ TypeResult Parser::ParseTypeFromString(StringRef TypeStr, StringRef Context, | |||
| return Result; | |||
| } | |||
|
|
|||
| ExprResult | |||
| Parser::ParseBoundsAttributeArgFromString(StringRef ExprStr, StringRef Context, | |||
There was a problem hiding this comment.
I am not sure how heavy weight this is, do you think it would make sense to only spin all of these up once per declaration instead of once per annotation? Feel free to leave as is for now but maybe we could leave a fixme or todo in case this shows up in a profile.
There was a problem hiding this comment.
I think it's tricky, because IIUC the Lexer is immutably tied to a FileID, which is tied to a read-only MemoryBuffer, so we have to construct new ones for each string. In ClangImporter we get around this by caching the parse result of e.g. the swift_attr strings. Of course, even if the string content we're parsing would be the exact same, the resulting DeclRefs would vary depending on the surrounding scope, so we'd have to flush the cache for each new declaration. I don't think we'd have enough cache hits for identical attribute arguments within a single declaration to warrant it.
| AttrName = "__sized_by_or_null"; | ||
| Kind = AttributeCommonInfo::AT_SizedByOrNull; | ||
| break; | ||
| case BoundsSafetyKind::EndedBy: |
There was a problem hiding this comment.
I think we should have a default branch with unreachable here.
There was a problem hiding this comment.
I prefer leaving it out for switches that are expected to be exhaustive, so that we get warnings if a new enum option is updated without updating the switch; -Wswitch doesn't fire when there is a default case. I can add a range assert before the switch.
|
@swift-ci please smoke test |
There was a problem hiding this comment.
Overall looks good to me. I think there might be a couple extra scenarios we might want to test. Particularly cases when the API already has bounds safety annotations but the same API is also annotated via APINotes. Do we get duplicated annotations? What if the annotations contradict each other? I think these scenarios can happen when someone is using APINotes to annotate 3rd party headers, but a newer version of those headers will introduce these annotations at the source.
|
@swift-ci please test |
Normally multiple annotations result in an error (and they do here also), however there are certain exceptions where e.g. a redeclaration is allowed to have the same attributes as the original declaration, provided that the attribute args match. Added an exception to let API notes have redundant attributes, again provided that they match. |
delcypher
added
the
clang:bounds-safety
| /* TO_UPSTREAM(BoundsSafety) ON */ | ||
| class BoundsSafetyInfo { | ||
| public: | ||
| enum class BoundsSafetyKind { |
There was a problem hiding this comment.
Doesn't this enum basically exist elsewhere? Should we be duplicating it here?
There was a problem hiding this comment.
Originally I was thinking about including __terminated_by, __single, __bidi_indexable etc, and I think there's still a possibility that those may be added later, especially __terminated_by. More importantly, this enum representation is now used in the binary encoding of API notes, so I think duplicating it is fine to keep them uncoupled.
This adds support for annotating function parameters with __counted_by, __sized_by, __counted_by_or_null, __sized_by_or_null, and __ended_by, using API notes. The main content of handlePtrCountedByEndedByAttr is extracted to applyPtrCountedByEndedByAttr and decoupled from ParsedAttr. The helper function ParseBoundsAttributeArgFromString is added to make it possible to parse count expressions from SemaAPINotes.
The current implementation of __terminated_by/__null_terminated makes it harder to extract from the iterative type processing, but since it doesn't require any extra context to parse the attribute, it can be applied using the normal Type override instead.
rdar://139830881