[BoundsSafety] make alloc_size imply __sized_by_or_null return type #10991
Conversation
hnrklssn
requested review from
apple-fcloutier,
delcypher,
patrykstefanski and
rapidsna
hnrklssn
force-pushed
the
alloc-size-sized-by-or-null
branch
from
7b8d848 to
f98c1fa
Compare
|
@swift-ci please test llvm |
hnrklssn
force-pushed
the
alloc-size-sized-by-or-null
branch
from
f98c1fa to
add268d
Compare
|
@swift-ci please test llvm |
|
The rest of the test failures seem unrelated. |
| assert(NewDCPTy->isOrNull() != T->isOrNull()); | ||
|
|
||
| if (!NewDCPTy->isOrNull()) | ||
| // Error already emitted for combining returns_nonnull with |
There was a problem hiding this comment.
Carrying over comment from previous PR:
In that case, would this function still be reached?
Yes: this happens for void * __sized_by_or_null(x * y) ignore_implicit_type_reverse_nullability(unsigned x, unsigned y) __attribute__((returns_nonnull)) __attribute__((alloc_size(1,2))). void * __sized_by_or_null(x * y) is first processed, and errors because of returns_nonnull. Later PostProcessBoundsSafetyAllocSizeAttribute runs and tries to construct an implicit __sized_by(x * y). Normally the implicit return type would be __sized_by_or_null, but with returns_nonnull it's __sized_by. The mismatch with the existing explicit attribute is what we run into here.
| return QualType(); // warning silenced by combining __sized_by and | ||
| // _Nonnull | ||
|
|
||
| S.Diag(Loc, |
There was a problem hiding this comment.
Carrying over comment from previous PR:
What would be the Loc for implicit __sized_by_or_null?
The loc points to the alloc_size attribute.
|
@swift-ci please test llvm |
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
TIL clang doesn't diagnose this already. Do you think this is a clang bug or is this expected for some reason?
There was a problem hiding this comment.
I think alloc_size just isn't expected to be used outside of system headers so the user experience for it probably hasn't been a priority
|
@swift-ci please test llvm |
| addAll(Other.begin(), Other.end()); | ||
| /* TO_UPSTREAM(BoundsSafety) ON | ||
| * Upstream uses addAll() instead, but that changes the attribute order. */ | ||
| addAllAtEnd(Other.begin(), Other.end()); |
There was a problem hiding this comment.
Could you elaborate what is happening here? Why do we need this change?
There was a problem hiding this comment.
This makes sure that attributes are always in the order they appear in source. Without this change you can sometimes have diagnostics that point out a "previous attribute" that actually appears after the first one.
There was a problem hiding this comment.
See https://github.com/swiftlang/llvm-project/pull/10991/files#diff-64ae08757f7769baa31fa57527708810f152e8aadcce69c4bed1ab03813c7bfcR23 for a test case that changed because of this. We now get
int var7 [[clang::internal_linkage]] __attribute__((common)); // expected-error{{'common' and 'clang::internal_linkage' attributes are not compatible}} \
that is <second-attr> and <first-attr> attributes are not compatible, which aligns with the case below it:
__attribute__((common)) int var8 [[clang::internal_linkage]]; // expected-error{{'clang::internal_linkage' and 'common' attributes are not compatible}} \
I can upstream this if we want to avoid diverging.
| } | ||
|
|
||
| /* TO_UPSTREAM(BoundsSafety) ON*/ | ||
| // FIXME: Rename to something more accurate |
There was a problem hiding this comment.
Only slightly related to your PR, but the whole thing is just checking if the parameter names of the dependent variables are the same in the new function. Not sure why we are using TreeTransform here. This can be just a loop over ->dependent_decls(). Though, I'm not saying we should address this in this PR.
|
@swift-ci please test llvm |
When -fbounds-safety is enabled, the semantics of the return value of functions is that of __sized_by_or_null. Unlike return types annotated with __sized_by_or_null, it is never included in the type system. Instead every relevant analysis and transformation has to specifically check for the alloc_size attribute. This patch infers a __sized_by_or_null return type for functions annotated with alloc_size. This enables us to remove those special cases and reduce complexity. rdar://118338657 rdar://91017404 rdar://11833865
Since the size expression for an implicit __sized_by_or_null from alloc_size can refer to anonymous parameters, it can be hard to figure out from the diagnostics what type annotation to use. Emitting a fix-its should make this clearer. This also helps in scenarios like: ``` int * __sized_by_or_null(count * size) foo(size_t count, size_t size); int * __sized_by_or_null(count * size) foo(size_t size, size_t count); ``` where diagnostics will point out that the return types don't match, but the spelled types are the same.
These parameters are only ever called with FunctionDecls, so no need to be generic (for now at least).
hnrklssn
force-pushed
the
alloc-size-sized-by-or-null
branch
from
1a6869c to
27d362a
Compare
|
@swift-ci please test llvm |
3 participants
When -fbounds-safety is enabled, the semantics of the return value of
functions is that of __sized_by_or_null. Unlike return types annotated
with __sized_by_or_null, it is never included in the type system.
Instead every relevant analysis and transformation has to specifically
check for the alloc_size attribute.
This patch infers a __sized_by_or_null return type for functions
annotated with alloc_size. This enables us to remove those special
cases and reduce complexity.
rdar://118338657
rdar://91017404
rdar://11833865