[Clang] Add __builtin_counted_by_ref builtin #102549
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Bill Wendling (bwendling) ChangesThe __builtin_get_counted_by builtin is used on a flexible array ptr = alloc(<ty>, COUNT); Full diff: https://github.com/llvm/llvm-project/pull/102549.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index b025a7681bfac..254cd157d5f9d 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -4774,3 +4774,9 @@ def ArithmeticFence : LangBuiltin<"ALL_LANGUAGES"> {
let Attributes = [CustomTypeChecking, Constexpr];
let Prototype = "void(...)";
}
+
+def GetCountedBy : Builtin {
+ let Spellings = ["__builtin_get_counted_by"];
+ let Attributes = [NoThrow];
+ let Prototype = "size_t*(void*)";
+}
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index d1af7fde157b6..58fc0dfe45e1b 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -27,6 +27,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/OSLog.h"
#include "clang/AST/OperationKinds.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/Basic/TargetBuiltins.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/Basic/TargetOptions.h"
@@ -2536,6 +2537,45 @@ static RValue EmitHipStdParUnsupportedBuiltin(CodeGenFunction *CGF,
return RValue::get(CGF->Builder.CreateCall(UBF, Args));
}
+namespace {
+
+/// MemberExprVisitor - Find the MemberExpr through all of the casts, array
+/// subscripts, and unary ops. This intentionally avoids all of them because
+/// we're interested only in the MemberExpr to check if it's a flexible array
+/// member.
+class MemberExprVisitor
+ : public ConstStmtVisitor<MemberExprVisitor, const Expr *> {
+public:
+ //===--------------------------------------------------------------------===//
+ // Visitor Methods
+ //===--------------------------------------------------------------------===//
+
+ const Expr *Visit(const Expr *E) {
+ return ConstStmtVisitor<MemberExprVisitor, const Expr *>::Visit(E);
+ }
+ const Expr *VisitStmt(const Stmt *S) { return nullptr; }
+
+ const Expr *VisitMemberExpr(const MemberExpr *E) { return E; }
+
+ const Expr *VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
+ return Visit(E->getBase());
+ }
+ const Expr *VisitCastExpr(const CastExpr *E) {
+ return Visit(E->getSubExpr());
+ }
+ const Expr *VisitParenExpr(const ParenExpr *E) {
+ return Visit(E->getSubExpr());
+ }
+ const Expr *VisitUnaryAddrOf(const clang::UnaryOperator *E) {
+ return Visit(E->getSubExpr());
+ }
+ const Expr *VisitUnaryDeref(const clang::UnaryOperator *E) {
+ return Visit(E->getSubExpr());
+ }
+};
+
+} // anonymous namespace
+
RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
const CallExpr *E,
ReturnValueSlot ReturnValue) {
@@ -3563,6 +3603,26 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
return RValue::get(emitBuiltinObjectSize(E->getArg(0), Type, ResType,
/*EmittedE=*/nullptr, IsDynamic));
}
+ case Builtin::BI__builtin_get_counted_by: {
+ llvm::Value *Result = llvm::ConstantPointerNull::get(
+ cast<llvm::PointerType>(ConvertType(E->getType())));
+
+ if (const Expr *Ptr = MemberExprVisitor().Visit(E->getArg(0))) {
+ const MemberExpr *ME = cast<MemberExpr>(Ptr);
+ bool IsFlexibleArrayMember = ME->isFlexibleArrayMemberLike(
+ getContext(), getLangOpts().getStrictFlexArraysLevel(),
+ /*IgnoreTemplateOrMacroSubstitution=*/false);
+
+ if (!ME->HasSideEffects(getContext()) && IsFlexibleArrayMember &&
+ ME->getMemberDecl()->getType()->isCountAttributedType()) {
+ const FieldDecl *FAMDecl = dyn_cast<FieldDecl>(ME->getMemberDecl());
+ if (const FieldDecl *CountFD = FindCountedByField(FAMDecl))
+ Result = GetCountedByFieldExprGEP(ME, FAMDecl, CountFD);
+ }
+ }
+
+ return RValue::get(Result);
+ }
case Builtin::BI__builtin_prefetch: {
Value *Locality, *RW, *Address = EmitScalarExpr(E->getArg(0));
// FIXME: Technically these constants should of type 'int', yes?
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index a1dce741c78a1..55cd95c08e3ff 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1100,15 +1100,7 @@ static bool getGEPIndicesToField(CodeGenFunction &CGF, const RecordDecl *RD,
return false;
}
-/// This method is typically called in contexts where we can't generate
-/// side-effects, like in __builtin_dynamic_object_size. When finding
-/// expressions, only choose those that have either already been emitted or can
-/// be loaded without side-effects.
-///
-/// - \p FAMDecl: the \p Decl for the flexible array member. It may not be
-/// within the top-level struct.
-/// - \p CountDecl: must be within the same non-anonymous struct as \p FAMDecl.
-llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
+llvm::Value *CodeGenFunction::GetCountedByFieldExprGEP(
const Expr *Base, const FieldDecl *FAMDecl, const FieldDecl *CountDecl) {
const RecordDecl *RD = CountDecl->getParent()->getOuterLexicalRecordContext();
@@ -1141,12 +1133,25 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
return nullptr;
Indices.push_back(Builder.getInt32(0));
- Res = Builder.CreateInBoundsGEP(
+ return Builder.CreateInBoundsGEP(
ConvertType(QualType(RD->getTypeForDecl(), 0)), Res,
RecIndicesTy(llvm::reverse(Indices)), "..counted_by.gep");
+}
- return Builder.CreateAlignedLoad(ConvertType(CountDecl->getType()), Res,
- getIntAlign(), "..counted_by.load");
+/// This method is typically called in contexts where we can't generate
+/// side-effects, like in __builtin_dynamic_object_size. When finding
+/// expressions, only choose those that have either already been emitted or can
+/// be loaded without side-effects.
+///
+/// - \p FAMDecl: the \p Decl for the flexible array member. It may not be
+/// within the top-level struct.
+/// - \p CountDecl: must be within the same non-anonymous struct as \p FAMDecl.
+llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
+ const Expr *Base, const FieldDecl *FAMDecl, const FieldDecl *CountDecl) {
+ if (llvm::Value *GEP = GetCountedByFieldExprGEP(Base, FAMDecl, CountDecl))
+ return Builder.CreateAlignedLoad(ConvertType(CountDecl->getType()), GEP,
+ getIntAlign(), "..counted_by.load");
+ return nullptr;
}
const FieldDecl *CodeGenFunction::FindCountedByField(const FieldDecl *FD) {
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 1c0a0e117e560..e5f5b94bba54b 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -3309,6 +3309,10 @@ class CodeGenFunction : public CodeGenTypeCache {
/// \p nullptr if either the attribute or the field doesn't exist.
const FieldDecl *FindCountedByField(const FieldDecl *FD);
+ llvm::Value *GetCountedByFieldExprGEP(const Expr *Base,
+ const FieldDecl *FAMDecl,
+ const FieldDecl *CountDecl);
+
/// Build an expression accessing the "counted_by" field.
llvm::Value *EmitLoadOfCountedByField(const Expr *Base,
const FieldDecl *FAMDecl,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8defc8e1c185c..33bc71d621ddd 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -28,6 +28,7 @@
#include "clang/AST/OperationKinds.h"
#include "clang/AST/ParentMapContext.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/AST/Type.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/Builtins.h"
@@ -6390,9 +6391,65 @@ ExprResult Sema::ActOnCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
currentEvaluationContext().ReferenceToConsteval.erase(DRE);
}
}
+
return Call;
}
+const FieldDecl *FindCountedByField(const FieldDecl *FD) {
+ if (!FD)
+ return nullptr;
+
+ const auto *CAT = FD->getType()->getAs<CountAttributedType>();
+ if (!CAT)
+ return nullptr;
+
+ const auto *CountDRE = cast<DeclRefExpr>(CAT->getCountExpr());
+ const auto *CountDecl = CountDRE->getDecl();
+ if (const auto *IFD = dyn_cast<IndirectFieldDecl>(CountDecl))
+ CountDecl = IFD->getAnonField();
+
+ return dyn_cast<FieldDecl>(CountDecl);
+}
+
+namespace {
+
+/// MemberExprVisitor - Find the MemberExpr through all of the casts, array
+/// subscripts, and unary ops. This intentionally avoids all of them because
+/// we're interested only in the MemberExpr to check if it's a flexible array
+/// member.
+class MemberExprVisitor
+ : public ConstStmtVisitor<MemberExprVisitor, const Expr *> {
+public:
+ //===--------------------------------------------------------------------===//
+ // Visitor Methods
+ //===--------------------------------------------------------------------===//
+
+ const Expr *Visit(const Expr *E) {
+ return ConstStmtVisitor<MemberExprVisitor, const Expr *>::Visit(E);
+ }
+ const Expr *VisitStmt(const Stmt *S) { return nullptr; }
+
+ const Expr *VisitMemberExpr(const MemberExpr *E) { return E; }
+
+ const Expr *VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
+ return Visit(E->getBase());
+ }
+ const Expr *VisitCastExpr(const CastExpr *E) {
+ return Visit(E->getSubExpr());
+ }
+ const Expr *VisitParenExpr(const ParenExpr *E) {
+ return Visit(E->getSubExpr());
+ }
+ const Expr *VisitUnaryAddrOf(const UnaryOperator *E) {
+ return Visit(E->getSubExpr());
+ }
+ const Expr *VisitUnaryDeref(const UnaryOperator *E) {
+ return Visit(E->getSubExpr());
+ }
+};
+
+} // anonymous namespace
+
ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
MultiExprArg ArgExprs, SourceLocation RParenLoc,
Expr *ExecConfig, bool IsExecConfig,
@@ -6590,8 +6647,36 @@ ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
VK_PRValue, RParenLoc, CurFPFeatureOverrides());
}
- return BuildResolvedCallExpr(Fn, NDecl, LParenLoc, ArgExprs, RParenLoc,
- ExecConfig, IsExecConfig);
+
+ Result = BuildResolvedCallExpr(Fn, NDecl, LParenLoc, ArgExprs, RParenLoc,
+ ExecConfig, IsExecConfig);
+
+ if (FunctionDecl *FDecl = dyn_cast_or_null<FunctionDecl>(NDecl);
+ FDecl && FDecl->getBuiltinID() == Builtin::BI__builtin_get_counted_by) {
+ if (const Expr *Ptr = MemberExprVisitor().Visit(ArgExprs[0])) {
+ const MemberExpr *ME = cast<MemberExpr>(Ptr);
+ bool IsFlexibleArrayMember = ME->isFlexibleArrayMemberLike(
+ Context, getLangOpts().getStrictFlexArraysLevel(),
+ /*IgnoreTemplateOrMacroSubstitution=*/false);
+
+ if (!ME->HasSideEffects(Context) && IsFlexibleArrayMember &&
+ ME->getMemberDecl()->getType()->isCountAttributedType()) {
+ const FieldDecl *FAMDecl = dyn_cast<FieldDecl>(ME->getMemberDecl());
+ if (const FieldDecl *CountFD = FindCountedByField(FAMDecl)) {
+ // The builtin returns a 'size_t *', however 'size_t' might not be
+ // the type of the count field. Thus we create an explicit c-style
+ // cast to ensure the proper types going forward.
+ QualType PtrTy = Context.getPointerType(CountFD->getType());
+ Result = CStyleCastExpr::Create(
+ Context, PtrTy, VK_LValue, CK_BitCast, Result.get(), nullptr,
+ FPOptionsOverride(), Context.CreateTypeSourceInfo(PtrTy),
+ LParenLoc, RParenLoc);
+ }
+ }
+ }
+ }
+
+ return Result;
}
Expr *Sema::BuildBuiltinCallExpr(SourceLocation Loc, Builtin::ID Id,
diff --git a/clang/test/CodeGen/builtin-get-counted-by.c b/clang/test/CodeGen/builtin-get-counted-by.c
new file mode 100644
index 0000000000000..8209db6a77111
--- /dev/null
+++ b/clang/test/CodeGen/builtin-get-counted-by.c
@@ -0,0 +1,83 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O2 -emit-llvm -o - %s | FileCheck %s --check-prefix=X86_64
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O2 -emit-llvm -o - %s | FileCheck %s --check-prefix=I386
+
+struct s {
+ char x;
+ short count;
+ int array[] __attribute__((counted_by(count)));
+};
+
+// X86_64-LABEL: define dso_local noalias noundef ptr @test1(
+// X86_64-SAME: i32 noundef [[SIZE:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// X86_64-NEXT: [[ENTRY:.*:]]
+// X86_64-NEXT: [[CONV:%.*]] = sext i32 [[SIZE]] to i64
+// X86_64-NEXT: [[MUL:%.*]] = shl nsw i64 [[CONV]], 2
+// X86_64-NEXT: [[ADD:%.*]] = add nsw i64 [[MUL]], 4
+// X86_64-NEXT: [[CALL:%.*]] = tail call ptr @malloc(i64 noundef [[ADD]]) #[[ATTR3:[0-9]+]]
+// X86_64-NEXT: [[CONV1:%.*]] = trunc i32 [[SIZE]] to i16
+// X86_64-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[CALL]], i64 2
+// X86_64-NEXT: store i16 [[CONV1]], ptr [[DOT_COUNTED_BY_GEP]], align 2, !tbaa [[TBAA2:![0-9]+]]
+// X86_64-NEXT: ret ptr [[CALL]]
+//
+// I386-LABEL: define dso_local noalias noundef ptr @test1(
+// I386-SAME: i32 noundef [[SIZE:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// I386-NEXT: [[ENTRY:.*:]]
+// I386-NEXT: [[MUL:%.*]] = shl i32 [[SIZE]], 2
+// I386-NEXT: [[ADD:%.*]] = add i32 [[MUL]], 4
+// I386-NEXT: [[CALL:%.*]] = tail call ptr @malloc(i32 noundef [[ADD]]) #[[ATTR3:[0-9]+]]
+// I386-NEXT: [[CONV:%.*]] = trunc i32 [[SIZE]] to i16
+// I386-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[CALL]], i32 2
+// I386-NEXT: store i16 [[CONV]], ptr [[DOT_COUNTED_BY_GEP]], align 2, !tbaa [[TBAA3:![0-9]+]]
+// I386-NEXT: ret ptr [[CALL]]
+//
+struct s *test1(int size) {
+ struct s *p = __builtin_malloc(sizeof(struct s) + sizeof(int) * size);
+
+ *__builtin_get_counted_by(p->array) = size;
+ *__builtin_get_counted_by(&p->array[0]) = size;
+ return p;
+}
+
+struct z {
+ char x;
+ short count;
+ int array[];
+};
+
+// X86_64-LABEL: define dso_local noalias noundef ptr @test2(
+// X86_64-SAME: i32 noundef [[SIZE:%.*]]) local_unnamed_addr #[[ATTR2:[0-9]+]] {
+// X86_64-NEXT: [[ENTRY:.*:]]
+// X86_64-NEXT: [[CONV:%.*]] = sext i32 [[SIZE]] to i64
+// X86_64-NEXT: [[MUL:%.*]] = shl nsw i64 [[CONV]], 2
+// X86_64-NEXT: [[ADD:%.*]] = add nsw i64 [[MUL]], 4
+// X86_64-NEXT: [[CALL:%.*]] = tail call ptr @malloc(i64 noundef [[ADD]]) #[[ATTR3]]
+// X86_64-NEXT: ret ptr [[CALL]]
+//
+// I386-LABEL: define dso_local noalias noundef ptr @test2(
+// I386-SAME: i32 noundef [[SIZE:%.*]]) local_unnamed_addr #[[ATTR2:[0-9]+]] {
+// I386-NEXT: [[ENTRY:.*:]]
+// I386-NEXT: [[MUL:%.*]] = shl i32 [[SIZE]], 2
+// I386-NEXT: [[ADD:%.*]] = add i32 [[MUL]], 4
+// I386-NEXT: [[CALL:%.*]] = tail call ptr @malloc(i32 noundef [[ADD]]) #[[ATTR3]]
+// I386-NEXT: ret ptr [[CALL]]
+//
+struct z *test2(int size) {
+ struct z *p = __builtin_malloc(sizeof(struct z) + sizeof(int) * size);
+
+ if (__builtin_get_counted_by(&p->array[0]))
+ *__builtin_get_counted_by(&p->array[0]) = size;
+
+ return p;
+}
+//.
+// X86_64: [[TBAA2]] = !{[[META3:![0-9]+]], [[META3]], i64 0}
+// X86_64: [[META3]] = !{!"short", [[META4:![0-9]+]], i64 0}
+// X86_64: [[META4]] = !{!"omnipotent char", [[META5:![0-9]+]], i64 0}
+// X86_64: [[META5]] = !{!"Simple C/C++ TBAA"}
+//.
+// I386: [[TBAA3]] = !{[[META4:![0-9]+]], [[META4]], i64 0}
+// I386: [[META4]] = !{!"short", [[META5:![0-9]+]], i64 0}
+// I386: [[META5]] = !{!"omnipotent char", [[META6:![0-9]+]], i64 0}
+// I386: [[META6]] = !{!"Simple C/C++ TBAA"}
+//.
diff --git a/clang/test/Sema/builtin-get-counted-by.c b/clang/test/Sema/builtin-get-counted-by.c
new file mode 100644
index 0000000000000..18cef35b0509a
--- /dev/null
+++ b/clang/test/Sema/builtin-get-counted-by.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct fam_struct {
+ char x;
+ short count;
+ int array[] __attribute__((counted_by(count)));
+} *p;
+
+struct non_fam_struct {
+ char x;
+ short count;
+ int array[];
+} *q;
+
+void foo(int size) {
+ *__builtin_get_counted_by(p->array) = size;
+
+ if (__builtin_get_counted_by(q->array))
+ *__builtin_get_counted_by(q->array) = size;
+
+ *__builtin_get_counted_by(p->count) = size; // expected-error{{incompatible integer to pointer conversion passing 'short' to parameter of type 'void *'}}
+}
|
|
|
|
There's still some cleanup to do. Firstly, the code to get the |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I'd expect some kind of diagnostic when the specified field doesn't have a corresponding counted_by field. |
So there's a complication with that. The use case for this builtin is to automatically set the The idea is to do something like: #define kmalloc(type, COUNT) ({ \
... \
if (__builtin_get_counted_by(__p->FAM)) \
*__builtin_get_counted_by(__p->FAM) = COUNT; \
__p; \
})We want the builtin to return a As you can see, this builtin has a very limited utility. Obviously, one wouldn't write: *__builtin_get_counted_by(ptr->FAM) = 42;when using (Note: I'll have to expand the commit message a lot. What's currently there is more-or-less a placeholder.) |
Yup, that's correct. There needs to be a way to programmatically determine what the "counter" variable is so that we don't have to create a separate (and ultimately redundant) allocator API for FAM structs that use "counted_by". It could be entirely automatic. Some more background details are here: |
|
So the idea here is that if the struct in question uses counted_by, you automatically set the count... and if, for whatever reason, the compiler can't find the corresponding field, you just throw away the count? That seems like an terrifying API; it's impossible to predict what it will do with a given bit of code. Maybe it makes sense to return null if you find the field, but it doesn't have a counted_by attribute at all. But there should still be diagnostics for all the other failure modes. |
I agree and it was mentioned in the GCC bug thread. This builtin will have very limited utility (none?) outside of the Linux kernel and it's
I agree, with one exception. We're planning on extending the |
bwendling
changed the title
|
I removed the WIP and updated the description. I'll look into adding some more diagnostics. |
There was a problem hiding this comment.
Hmm, I could see how this could be useful, but it seems like a rather niche use case.
@AaronBallman Should this go through the ususal process of someone writing an RFC for it seeing as it is a language extension?
I also saw that there’s a GCC bug report (or something like that) about this. Is it clear at this point what the GCC devs’ stance on adding this as a builtin is?
As for the implementation: I’m not really too familiar w/ codegen, so I can’t comment on that part (it seems we’re maybe doing a bit too much checking in codegen as opposed to in sema, but that might just be me). I don’t think the way we’re handling this in Sema is quite right tho atm.
clang/lib/Sema/SemaExpr.cpp
Outdated
| bool IsFlexibleArrayMember = ME->isFlexibleArrayMemberLike( | ||
| Context, getLangOpts().getStrictFlexArraysLevel()); | ||
|
|
||
| if (!ME->HasSideEffects(Context) && IsFlexibleArrayMember && |
There was a problem hiding this comment.
I don’t think we need to reject this if the expression contains side-effects, but we should definitely warn that it isn’t actually going to be evaluated. I think something along the lines of ‘argument to '__builtin_get_counted_by' has side-effects that will be discarded’ (which is pretty much exactly what we used to issue for e.g. __builtin_assume before we had to change that for... reasons), would be enough to tell the user ‘hey, make sure you really want to do this’.
There was a problem hiding this comment.
Oh, also, this should issue a diagnostic (probably honestly just straight-up an error) if the expression isn’t a member access of any sort. Or is there a use case where this might be applied to something that isn’t even a member expression?
There was a problem hiding this comment.
I don’t think we need to reject this if the expression contains side-effects, but we should definitely warn that it isn’t actually going to be evaluated.
Thinking about this again, it’s actually a bit more complicated than that: this only applies if we return null (we don’t evaluate it at all if the expression names a field that isn’t a FAM guarded by a counted_by attribute). If we don’t return null, then the expression is just evaluated normally.
Also as a side note, Clang is very conservative as to what counts as a ‘side effect’, so even a call to e.g.
S* foo(S* s) { return s; }has side effects, because function calls in general can have side effects and we don’t try very hard (or not at all, actually) to figure out whether a language construct that could have side effects does or doesn’t have side effects. Iirc HasSideEffects only ever returns false if it’s something that can never have side effects (like builtin operators).
There was a problem hiding this comment.
we don’t evaluate it at all
Actually, we should probably evaluate it regardless; that’s probably a more sensible approach and it means we can do away with this entire side effect discussion entirely. If we end up returning null and the expression doesn’t have side effects, the optimiser will just remove it anyway, so there’s no reason to deal w/ that here.
|
Also, as ever, I forgot to mention this, but this also needs a release note and it should also go in the docs next to all the other builtins ( |
I'll be happy to open this up to the larger community and produce an RFC. However, because of its niche usage it might not garner much attention. I don't personally consider this a language extension only a new builtin, but I'm probably in the minority. (And really it's just a matter of nomenclature.)
I got the impression that the GCC developers are going forward with this builtin.
Happy to rectify any issues in Sema and CodeGen. For the record, I'm doing roughly the same number of checks in both places, but in Sema I don't have to deal with code generation, so I can overlook some things I would normally have to consider (waves hands). |
bwendling
requested review from
cyndyishida,
rupprecht,
keith,
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci,
ftynse and
makslevental
as code owners
|
@rapidsna @AaronBallman @hnrklssn There's a comment in _Generic(
__builtin_counted_by_ref(q->array),
void *: (void)0,
default: *__builtin_counted_by_ref(q->array) = 42
);Results in: when |
Just to make sure, is the following having any effect if you declare it in the loop before calling Or are you wondering about some more technical detail? |
I saw that, but it doesn't appear to work like it should. It seems to want a |
Hmm, where does it want a CFGBlock? The parser isn't my field of expertise, but I'm happy to lend a second pair of eyes. |
That sounds... odd, but @AaronBallman probably knows more about this. |
It wants it when issuing warnings. See in // Emit delayed diagnostics.
if (!fscope->PossiblyUnreachableDiags.empty()) {
bool analyzed = false;
// Register the expressions with the CFGBuilder.
for (const auto &D : fscope->PossiblyUnreachableDiags) {
for (const Stmt *S : D.Stmts)
AC.registerForcedBlockExpression(S);
}
if (AC.getCFG()) {
analyzed = true;
for (const auto &D : fscope->PossiblyUnreachableDiags) {
bool AllReachable = true;
for (const Stmt *S : D.Stmts) {
const CFGBlock *block = AC.getBlockForRegisteredExpression(S);
CFGReverseBlockReachabilityAnalysis *cra =
AC.getCFGReachablityAnalysis();
// FIXME: We should be able to assert that block is non-null, but
// the CFG analysis can skip potentially-evaluated expressions in
// edge cases; see test/Sema/vla-2.c.
if (block && cra) {
// Can this block be reached from the entrance?
if (!cra->isReachable(&AC.getCFG()->getEntry(), block)) {
AllReachable = false;
break;
}
}
// If we cannot map to a basic block, assume the statement is
// reachable.
}
if (AllReachable)
S.Diag(D.Loc, D.PD);
}
}
if (!analyzed)
flushDiagnostics(S, fscope);
}The |
I tried dumping the CFG for a simple _Generic expression, and it doesn't contain blocks for each branch, only the actually taken one. So I think special handling for _Generic is needed here, since it currently assumes all expressions without blocks are reachable. |
Okay, so then how do I determine if the current |
Try using |
Thanks for the hint. I'm having some issues though. The use of this construct: EnterExpressionEvaluationContext PotentiallyEvaluatedIfUsed(
Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed);does allow me to omit runtime diagnostics. However, there are many it doesn't catch. With this code: struct non_fam_struct {
char x;
short count;
int array[];
} *q;
_Generic(
__builtin_counted_by_ref(q->array),
void *: (void)0,
default: *__builtin_counted_by_ref(q->array) = 42
);I get this output: These two diagnostics aren't handled through the So, what I think I need is a way to gather up all diagnostics that occur within the |
| @@ -1476,6 +1476,10 @@ def NoDeref : DiagGroup<"noderef">; | |||
| def BoundsSafetyCountedByEltTyUnknownSize : | |||
| DiagGroup<"bounds-safety-counted-by-elt-type-unknown-size">; | |||
|
|
|||
| // counted_by warnings | |||
| def CountedByAttribute | |||
There was a problem hiding this comment.
CountedByAttribute seems misleadingly generic compared to get-counted-by-side-effects, which is very specific.
These diagnostics look like they're in the reachable |
The Here's the whole program: struct fam_struct {
int x;
struct {
struct {
struct {
char count;
};
};
};
struct {
int array[] __attribute__((counted_by(count)));
};
} *p;
struct non_fam_struct {
char x;
short count;
int array[];
} *q;
void g(char *);
void *alloc_s(int size) {
_Generic(
__builtin_counted_by_ref(q->array),
void *: (void)0,
default: *__builtin_counted_by_ref(q->array) = 42
); // ok
return (void *)0;
} |
Ah right, I see. Perhaps rather than updating every diagnostic to check whether it's reachable, there might be a way to perform the check in |
If that's too hacky, then I could potentially have it return the type |
|
Just fyi, iirc Aaron mentioned something about being unavailable for a week or two, so you might have to wait a bit until you get an answer from him. |
Thanks for the Info! @bwendling: I'd suggest implementing the DiagnosticEngine workaround for now, as it seems reasonable to have a generic approach rather than every diagnostic checking for reachability for something as (relatively) obscure as _Generic. Hopefully it'll only require small adjustments, if any. |
Yeah. It would be very difficult to weed through all possible diagnostics to isolate the ones that we can "safely ignore" as opposed to those we can't. I'll go with the |
|
This should be ready for a final review. PTAL. |
clang/docs/LanguageExtensions.rst
Outdated
| ``counted_by`` attribute. | ||
|
|
||
| The argument must be a pointer to a flexible array member. If the argument | ||
| isn't a flexible array member or doesn't have the ``counted_by`` attribute, the |
There was a problem hiding this comment.
It might worth nothing that "a flexible array member can only have counted_by and having a different bounds attribute (e.g., counted_by_or_null, sized_by_or_null, or sized_by) on a flexible array member is an error."
There was a problem hiding this comment.
I'm not sure I agree that it's an error. I just want to convey that if counted_by isn't specified then it returns NULL.
There was a problem hiding this comment.
Well, what I mean is that this is actually an error currently to add any other bounds attributes on a flexible array member, because other attributes don't make sense. For example, for counted_by_or_null, a flexible array member cannot be a null because it's not a pointer. And sized_by* is only allowed on pointers. Hence, these are naturally prevented right now, and if we extend the builtin to pointers we may want to emit an error for sized_by* because it has a different scale than counted_by*, but we can discuss that later when we actually support it.
There was a problem hiding this comment.
But is this the best place to talk about which attributes are valid on a flexible array member? IMHO, that comment should be closer to the documentation for those attributes; in a similar vein, the checks for those situations should be where the attributes are handled during parsing.
There was a problem hiding this comment.
I thought it would be relevant in the interest for this builtin's behavior for other bounds attributes. We would want to define the behavior for them in the future when this builtin gets exposed to pointers where these attributes are possible (which would be basically to decide whether we'd like to diagnose it or ignore them silently), but it's not currently exposed yet.
There was a problem hiding this comment.
Okay, do you have an example of what to add? I'm coming up with clunky ways of saying it. 😄
There was a problem hiding this comment.
Here's the latest. I addressed most of your comments. There are a couple that I wasn't able to, because the builtin had been replaced with a direct access of the counter by that time. If you have any suggestions for how to get around that, please let me know.
clang/docs/LanguageExtensions.rst
Outdated
| ``counted_by`` attribute. | ||
|
|
||
| The argument must be a pointer to a flexible array member. If the argument | ||
| isn't a flexible array member or doesn't have the ``counted_by`` attribute, the |
There was a problem hiding this comment.
I'm not sure I agree that it's an error. I just want to convey that if counted_by isn't specified then it returns NULL.
clang/lib/AST/Decl.cpp
Outdated
| @@ -3652,6 +3652,10 @@ unsigned FunctionDecl::getBuiltinID(bool ConsiderWrapperFunctions) const { | |||
| (!hasAttr<ArmBuiltinAliasAttr>() && !hasAttr<BuiltinAliasAttr>())) | |||
| return 0; | |||
|
|
|||
| if (getASTContext().getLangOpts().CPlusPlus && | |||
clang/include/clang/AST/Decl.h
Outdated
| @@ -3208,7 +3208,7 @@ class FieldDecl : public DeclaratorDecl, public Mergeable<FieldDecl> { | |||
|
|
|||
| /// Find the FieldDecl specified in a FAM's "counted_by" attribute. Returns | |||
| /// \p nullptr if either the attribute or the field doesn't exist. | |||
| const FieldDecl *findCountedByField() const; | |||
| FieldDecl *findCountedByField() const; | |||
There was a problem hiding this comment.
Okay, so this can go back to returning a const FieldDecl *?
The usual pattern is to have the const member function return a const pointer, and have a non-const overload which returns a non-const pointer.
clang/lib/AST/Decl.cpp
Outdated
| @@ -3652,6 +3652,10 @@ unsigned FunctionDecl::getBuiltinID(bool ConsiderWrapperFunctions) const { | |||
| (!hasAttr<ArmBuiltinAliasAttr>() && !hasAttr<BuiltinAliasAttr>())) | |||
| return 0; | |||
|
|
|||
| if (getASTContext().getLangOpts().CPlusPlus && | |||
There was a problem hiding this comment.
The only tests I see are for C, did I miss this one?
| int global_array[42]; | ||
| int global_int; | ||
|
|
||
| struct fam_struct { |
There was a problem hiding this comment.
Another kind of FAM struct we should probably test is:
struct erp {
int count;
int fam[][12]; __attribute__((counted_by(count)));
};
There was a problem hiding this comment.
That's not a valid flexible array member, so the attribute should be rejected from it.
bwendling
force-pushed
the
builtin_get_counted_by
branch
from
44ff61f to
51a4f31
Compare
|
Crap!!! This somehow got closed. I hate Git >:-( I'll try to reinstate this. |
|
Wasn't supposed to be deleted. |
|
I'm sorry. I had to create a new PR. Here it is: There's a change in there that's yet to be completed (the CGBuiltins.cpp change). |
The __builtin_counted_by_ref builtin is used on a flexible array
pointer and returns a pointer to the "counted_by" attribute's COUNT
argument, which is a field in the same non-anonymous struct as the
flexible array member. This is useful for automatically setting the
count field without needing the programmer's intervention. Otherwise
it's possible to get this anti-pattern:
To prevent this anti-pattern, the user can create an allocator that
automatically performs the assignment:
The builtin's behavior is heavily dependent upon the "counted_by"
attribute existing. It's main utility is during allocation to avoid
the above anti-pattern. If the flexible array member doesn't have that
attribute, the builtin becomes a no-op. Therefore, if the flexible
array member has a "count" field not referenced by "counted_by", it
must be set explicitly after the allocation as this builtin will
return a "nullptr" and the assignment will most likely be elided.