[Clang] Allow malloc for functions returning structs #165433
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Aleksandr Nogikh (a-nogikh) ChangesThese attributes restrict the possible function signatures to the ones returning a pointer, which is not the case for some non-standard allocation functions variants. For example, P0901R11 proposed ::operator new overloads that return a return_size_t result - a struct that contains a pointer to the allocated memory as well as the actual size of the allocated memory. Another example is __size_returning_new. Relax the mentioned restriction and allow "malloc" and "alloc_size" attributes to be applied not just to the functions returning pointer types, but also to the functions returning records whose first member is a pointer (which would be assumed to point to the allocated memory). This is the case for return_size_t as well as std::span, should it be returned from such an annotated function. As the previous restriction was dictated by the need to set the noalias attribute, adjust the code generation logic to only assign the attribute when the returned value type is a pointer type. In future commits, codegen can be improved to recognize the noalias-ness of the pointer returned inside a span-like struct. This change also helps unlock the alloc token instrumentation for such non-standard allocation functions: Full diff: https://github.com/llvm/llvm-project/pull/165433.diff 7 Files Affected:
diff --git a/clang/include/clang/AST/TypeBase.h b/clang/include/clang/AST/TypeBase.h
index f07861f50fe8c..3ff66ddbf7edc 100644
--- a/clang/include/clang/AST/TypeBase.h
+++ b/clang/include/clang/AST/TypeBase.h
@@ -2601,6 +2601,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
bool isFunctionProtoType() const { return getAs<FunctionProtoType>(); }
bool isPointerType() const;
bool isPointerOrReferenceType() const;
+ bool isSpanLikeType() const;
bool isSignableType(const ASTContext &Ctx) const;
bool isSignablePointerType() const;
bool isSignableIntegerType(const ASTContext &Ctx) const;
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 4548af17e37f2..9c59640c6ca49 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -5753,3 +5753,23 @@ StringRef PredefinedSugarType::getName(Kind KD) {
}
llvm_unreachable("unexpected kind");
}
+
+bool Type::isSpanLikeType() const {
+ // Check that the type is a plain record with the first field being a pointer
+ // type and the second field being an integer.
+ // This matches the common implementation of std::span or sized_allocation_t
+ // in P0901R11.
+ const RecordDecl *RD = getAsRecordDecl();
+ if (!RD || RD->isUnion())
+ return false;
+ const RecordDecl *Def = RD->getDefinition();
+ if (!Def)
+ return false; // This is an incomplete type.
+ auto FieldsBegin = Def->field_begin();
+ if (std::distance(FieldsBegin, Def->field_end()) != 2)
+ return false;
+ const FieldDecl *FirstField = *FieldsBegin;
+ const FieldDecl *SecondField = *std::next(FieldsBegin);
+ return FirstField->getType()->isAnyPointerType() &&
+ SecondField->getType()->isIntegerType();
+}
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 465f3f4e670c2..6c3aab528dc47 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2507,7 +2507,8 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
}
if (const auto *RA = TargetDecl->getAttr<RestrictAttr>();
- RA && RA->getDeallocator() == nullptr)
+ RA && RA->getDeallocator() == nullptr &&
+ FI.getReturnType()->getAs<PointerType>())
RetAttrs.addAttribute(llvm::Attribute::NoAlias);
if (TargetDecl->hasAttr<ReturnsNonNullAttr>() &&
!CodeGenOpts.NullPointerIsValid)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 964a2a791e18f..9f1943fd7f5a0 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -571,7 +571,7 @@ static void handleAllocSizeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
assert(isFuncOrMethodForAttrSubject(D) && hasFunctionProto(D));
QualType RetTy = getFunctionOrMethodResultType(D);
- if (!RetTy->isPointerType()) {
+ if (!RetTy->isPointerType() && !RetTy->isSpanLikeType()) {
S.Diag(AL.getLoc(), diag::warn_attribute_return_pointers_only) << AL;
return;
}
@@ -1750,7 +1750,8 @@ static void handleTLSModelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
static void handleRestrictAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
QualType ResultType = getFunctionOrMethodResultType(D);
- if (!ResultType->isAnyPointerType() && !ResultType->isBlockPointerType()) {
+ if (!ResultType->isAnyPointerType() && !ResultType->isBlockPointerType() &&
+ !ResultType->isSpanLikeType()) {
S.Diag(AL.getLoc(), diag::warn_attribute_return_pointers_only)
<< AL << getFunctionOrMethodResultSourceRange(D);
return;
diff --git a/clang/test/CodeGen/attr-malloc.c b/clang/test/CodeGen/attr-malloc.c
index e69f8bce55f3b..238ca95a13be1 100644
--- a/clang/test/CodeGen/attr-malloc.c
+++ b/clang/test/CodeGen/attr-malloc.c
@@ -3,10 +3,17 @@
int *Mem;
void dealloc(int*);
+typedef struct {
+ void *p;
+ int size;
+} sized_ptr;
+
__attribute__((malloc)) int *MallocFunc(){ return Mem;}
// CHECK: define[[BEFORE:.*]] noalias[[AFTER:.*]]@MallocFunc
-// Ensure these two do not generate noalias here.
+// Ensure these three do not generate noalias here.
__attribute__((malloc(dealloc))) int *MallocFunc2(){ return Mem;}
// CHECK: define[[BEFORE]][[AFTER]]@MallocFunc2
__attribute__((malloc(dealloc, 1))) int *MallocFunc3(){ return Mem;}
// CHECK: define[[BEFORE]][[AFTER]]@MallocFunc3
+__attribute__((malloc)) sized_ptr MallocFunc4(){ return (sized_ptr){ .p = Mem };}
+// CHECK: define[[BEFORE]] { ptr, i32 } @MallocFunc4
diff --git a/clang/test/Sema/alloc-size.c b/clang/test/Sema/alloc-size.c
index 93714894a630a..29681fd483f41 100644
--- a/clang/test/Sema/alloc-size.c
+++ b/clang/test/Sema/alloc-size.c
@@ -30,6 +30,14 @@ void *KR() __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attr
void *(__attribute__((alloc_size(1))) * func_ptr1)(int);
void *(__attribute__((alloc_size(1, 2))) func_ptr2)(int, int);
+// Applying alloc_size to functions returning a struct with a pointer as a first field should work.
+typedef struct {
+ void* p;
+ int n;
+} sized_ptr;
+
+sized_ptr sized_ptr_alloc(int len) __attribute__((alloc_size(1)));
+
// TODO: according to GCC documentation the following should actually be the type
// “pointer to pointer to alloc_size attributed function returning void*” and should
// therefore be supported
diff --git a/clang/test/Sema/attr-malloc.c b/clang/test/Sema/attr-malloc.c
index a431aa43969d7..e65787bbc3f6a 100644
--- a/clang/test/Sema/attr-malloc.c
+++ b/clang/test/Sema/attr-malloc.c
@@ -13,6 +13,32 @@ int returns_int (void) __attribute((malloc)); // expected-warning {{attribut
int * returns_intptr(void) __attribute((malloc)); // no-warning
typedef int * iptr;
iptr returns_iptr (void) __attribute((malloc)); // no-warning
+typedef struct {
+ void *ptr;
+ size_t n;
+} sized_ptr;
+sized_ptr returns_sized_ptr (void) __attribute((malloc)); // no-warning
+
+// The first struct field must be pointer and the second must be an integer.
+// Check the possible ways to violate it.
+typedef struct {
+ size_t n;
+ void *ptr;
+} invalid_span1;
+invalid_span1 returns_non_std_span1 (void) __attribute((malloc)); // expected-warning {{attribute only applies to return values that are pointers}}
+
+typedef struct {
+ void *ptr;
+ void *ptr2;
+} invalid_span2;
+invalid_span2 returns_non_std_span2 (void) __attribute((malloc)); // expected-warning {{attribute only applies to return values that are pointers}}
+
+typedef struct {
+ void *ptr;
+ size_t n;
+ size_t n2;
+} invalid_span3;
+invalid_span3 returns_non_std_span3 (void) __attribute((malloc)); // expected-warning {{attribute only applies to return values that are pointers}}
__attribute((malloc)) void *(*f)(void); // expected-warning{{attribute only applies to functions}}
__attribute((malloc)) int (*g)(void); // expected-warning{{attribute only applies to functions}}
|
|
This LGTM, but I'd like to hear from others more familiar with the history of these attributes. In particular, we'd be diverging from GCC's behaviour, so that users would now need to do: IMHO, that's reasonable. Maybe we can ask GCC folks to also permit these attributes on functions that return span-like objects. |
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Needs release note. Needs documentation update for the attribute.
Since this attribute was originally defined by gcc, we should consult with gcc devs to see if they have a position on this. It would be a proble if gcc chooses some different behavior here.
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Since this attribute was originally defined by gcc, we should consult with gcc devs to see if they have a position on this. It would be a proble if gcc chooses some different behavior here.
+1 to this request
clang/lib/AST/Type.cpp
Outdated
| llvm_unreachable("unexpected kind"); | ||
| } | ||
|
|
||
| bool Type::isSpanLikeType() const { |
There was a problem hiding this comment.
I'm rather worried about this as a heuristic on Type. Consider:
struct Person {
const char *Name;
int Age;
};
struct Record {
void *allocator_function;
int reserved;
};
and so on. There are far too many reasons for a type to have a pointer and an integer that have nothing to do with being span-like.
I think this logic should be sunk into SemaDeclAttr.cpp to make it clear this is only valuable for the attributes in question, and should probably have a comment about false positives being expected.
There was a problem hiding this comment.
Hmm, yes, indeed, there are pointer+int structs that are not span-like at all.
I've moved the logic to SemaDeclAttr.cpp and added a note in the comments.
|
Is there a reason why a new attribute name can't be used instead? Like say |
Actually it is worse you would need to do something like: if you want to support older clang. |
|
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86488 for reference. |
…g structs These attributes restrict the possible function signatures to the ones returning a pointer, which is not the case for some non-standard allocation functions variants. For example, P0901R11 proposed ::operator new overloads that return a return_size_t result - a struct that contains a pointer to the allocated memory as well as the actual size of the allocated memory. Another example is __size_returning_new. Relax the mentioned restriction and allow "malloc" and "alloc_size" attributes to be applied not just to the functions returning pointer types, but also to the functions returning records whose first member is a pointer (which would be assumed to point to the allocated memory). This is the case for return_size_t as well as std::span, should it be returned from such an annotated function. As the previous restriction was dictated by the need to set the noalias attribute, adjust the code generation logic to only assign the attribute when the returned value type is a pointer type. In future commits, codegen can be improved to recognize the noalias-ness of the pointer returned inside a span-like struct. This change also helps unlock the alloc token instrumentation for such non-standard allocation functions: https://clang.llvm.org/docs/AllocToken.html#instrumenting-non-standard-allocation-functions
Also leave a comment about possible false positives.
Extending the range of functions alloc_size applies to makes no sense without the __builtin_object_size-side support. For now, just the malloc attribute changes should be enough.
Mention the change in the release notes and extend the attribute descriptions.
a-nogikh
force-pushed
the
features/relax-malloc-1
branch
from
bfb8b0b to
63d9cc8
Compare
a-nogikh
changed the title
|
Thank you for the feedback!
If adding a new |
I think that's a reasonable idea; I'd be happy with |
|
A pull request with the |
6 participants
The malloc attribute restricts the possible function signatures to the ones returning a pointer, which is not the case for some non-standard allocation functions variants. For example, P0901R11 proposed ::operator new overloads that return a return_size_t result - a struct that contains a pointer to the allocated memory as well as the actual size of the allocated memory. Another example is __size_returning_new.
Relax the mentioned restriction and allow the "malloc" attribute to be applied not just to the functions returning pointer types, but also to the functions returning records whose first member is a pointer (which would be assumed to point to the allocated memory). This is the case for return_size_t as well as std::span, should it be returned from such an annotated function.
As the previous restriction was dictated by the need to set the noalias attribute, adjust the code generation logic to only assign the attribute when the returned value type is a pointer type.
In future commits, codegen can be improved to recognize the noalias-ness of the pointer returned inside a span-like struct.
This change also helps unlock the alloc token instrumentation for such non-standard allocation functions:
https://clang.llvm.org/docs/AllocToken.html#instrumenting-non-standard-allocation-functions