[BoundsSafety] Support __single and __unsafe_indexable in template definitions in attribute only mode#10779
Conversation
delcypher
requested review from
hnrklssn,
j-hui,
patrykstefanski,
rapidsna and
ziqingluo-90
delcypher
added
the
clang:bounds-safety
|
@swift-ci test llvm |
|
@rapidsna I think you may have already come to this conclusion but my experience working on this patch suggests that all of our attributes should be type sugar rather than being embedded in |
|
@patrykstefanski I did look at doing this for
|
There was a problem hiding this comment.
There may be other cases, but if the code was ever modified so that -fbounds-safety could reach here, I think implicitly single pointers would probably miss spelling
There was a problem hiding this comment.
Got it. When I made the C++ example work with full -fbounds-safety it seems we went down this path but I didn't look into why.
There was a problem hiding this comment.
Why only in attribute-only mode?
There was a problem hiding this comment.
Because in that mode __single and __unsafe_indexable are sugar types. In -fbounds-safety that information is embedded in PointerType instead. Given that there's no good way to make this work in -fbounds-safety right now I decided to limit this to attribute-only mode. This seems like the right call given that it's the only one that really matters right now.
There was a problem hiding this comment.
I'm not sure we need to implement this for -fbounds-safety now, but I'm not sure that there isn't any good way to make that work.
There was a problem hiding this comment.
Ok there is a way. As you outlined elsewhere we could represent __single and __unsafe_indexable as sugar types in the AST for uninstantiated templates and then at template instantiation time rewrite the AST so that the sugar is dropped and the appropriate attribute is set in the PointerType.
This doesn't seem good to me because we'd be using two different representations for "is single" in the AST which might lead to bugs. Probably more importantly, I don't think this is an important problem to solve right now.
What we do for |
|
I think we discussed this offline. But, allowing those attributes to be only used on pointers wouldn't work? That is, |
We could do that but this problem seems simple enough to fix that I figured we should just do it. |
|
@swift-ci test |
|
@swift-ci test llvm |
There was a problem hiding this comment.
good shout, this here is firmly in the arcane realm
hnrklssn
left a comment
hnrklssn
left a comment
There was a problem hiding this comment.
Thanks for the thorough testing!
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
Swift Test for macOS failure looks unrelated. |
|
I'm going to squash and merge. |
…finitions in attribute only mode
Previously code like this would fail to compile:
```
template <class T>
class Ref {
public:
T __single ptr;
};
void use() {
Ref<int*> ok;
}
```
with the following error
```
error: '__single' attribute only applies to pointer arguments
```
This happens because we try to do type checking on the uninstantiated
template which fails because `T` is a `TemplateTypeParmType` and not a
`PointerType`.
In `-fbounds-safety` mode there's no good way to fix this because
`__single` and `__unsafe_indexable` are part of the `PointerType` so the
attribute can't be represented in the AST because there is no
`PointerType`, only a `TemplateTypeParmType`.
However, in `-fexperimental-bounds-safety-attributes` mode `__single`
and `__unsafe_indexable` are represented as type sugar (`AttributedType`).
This means it is possible to add the AttributedType on the
`TemplateTypeParmType` (i.e. adding sugar to the unknown template type).
This patch makes the above code compile in
`-fexperimental-bounds-safety-attributes` mode by
* Removing the error diagnostic when the type being checked in
`HandleBoundsSafetyPointerTypeAttr(...)` is a `TemplateTypeParmType`
and we are in `-fexperimental-bounds-safety-attributes` mode.
* Adding `ConstructBoundsSafetyPointerType::VisitTemplateTypeParmTypeLoc`
so that the `__single` or `__unsafe_indexable` type sugar is applied
when in `-fexperimental-bounds-safety-attributes` mode.
* Teaching `TreeTransform<Derived>::TransformAttributedType` to check that
`__single` and `__unsafe_indexable` are being applied to `PointerType`.
This method is used by the `TemplateInstantiator` class which performs
template instantiation.
In principle a similar approach could be used to support
`__null_terminated` in template definitions. However, doing so would be
quite complicated and should be handled separately (rdar://152451848).
rdar://152249884
delcypher
force-pushed
the
dliew/rdar-152249884
branch
from
633c528 to
1f11772
Compare
3 participants
Previously code like this would fail to compile:
with the following error
This happens because we try to do type checking on the uninstantiated template which fails because
Tis aTemplateTypeParmTypeand not aPointerType.In
-fbounds-safetymode there's no good way to fix this because__singleand__unsafe_indexableare part of thePointerTypeso the attribute can't be represented in the AST because there is noPointerType, only aTemplateTypeParmType.However, in
-fexperimental-bounds-safety-attributesmode__singleand__unsafe_indexableare represented as type sugar (AttributedType). This means it is possible to add the AttributedType on theTemplateTypeParmType(i.e. adding sugar to the unknown template type).This patch makes the above code compile in
-fexperimental-bounds-safety-attributesmode byRemoving the error diagnostic when the type being checked in
HandleBoundsSafetyPointerTypeAttr(...)is aTemplateTypeParmTypeand we are in-fexperimental-bounds-safety-attributesmode.Adding
ConstructBoundsSafetyPointerType::VisitTemplateTypeParmTypeLocso that the__singleor__unsafe_indexabletype sugar is applied when in-fexperimental-bounds-safety-attributesmode.Teaching
TreeTransform<Derived>::TransformAttributedTypeto check that__singleand__unsafe_indexableare being applied toPointerType. This method is used by theTemplateInstantiatorclass which performs template instantiation.In principle a similar approach could be used to support
__null_terminatedin template definitions. However, doing so would be quite complicated and should be handled separately (rdar://152451848).rdar://152249884