[CodeGen] Revamp counted_by calculations #70606
Conversation
Break down the counted_by calculations so that they correctly handle anonymous structs, which are specified internally as IndirectFieldDecls. Also simplify the code to use helper methods to get the field referenced by counted_by and the flexible array member itself, which also had some issues with FAMs in sub-structs.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Bill Wendling (bwendling) ChangesBreak down the counted_by calculations so that they correctly handle Full diff: https://github.com/llvm/llvm-project/pull/70606.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index dce5ee5888c458e..acee2c1af1ab368 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -859,53 +859,60 @@ CodeGenFunction::emitBuiltinObjectSize(const Expr *E, unsigned Type,
}
if (IsDynamic) {
- LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
- getLangOpts().getStrictFlexArraysLevel();
- const Expr *Base = E->IgnoreParenImpCasts();
-
- if (FieldDecl *FD = FindCountedByField(Base, StrictFlexArraysLevel)) {
- const auto *ME = dyn_cast<MemberExpr>(Base);
- llvm::Value *ObjectSize = nullptr;
-
- if (!ME) {
- const auto *DRE = dyn_cast<DeclRefExpr>(Base);
- ValueDecl *VD = nullptr;
-
- ObjectSize = ConstantInt::get(
- ResType,
- getContext().getTypeSize(DRE->getType()->getPointeeType()) / 8,
- true);
-
- if (auto *RD = DRE->getType()->getPointeeType()->getAsRecordDecl())
- VD = RD->getLastField();
-
- Expr *ICE = ImplicitCastExpr::Create(
- getContext(), DRE->getType(), CK_LValueToRValue,
- const_cast<Expr *>(cast<Expr>(DRE)), nullptr, VK_PRValue,
- FPOptionsOverride());
- ME = MemberExpr::CreateImplicit(getContext(), ICE, true, VD,
- VD->getType(), VK_LValue, OK_Ordinary);
- }
-
- // At this point, we know that \p ME is a flexible array member.
- const auto *ArrayTy = getContext().getAsArrayType(ME->getType());
+ // The code generated here calculates the size of a struct with a flexible
+ // array member that uses the counted_by attribute. There are two instances
+ // we handle:
+ //
+ // struct s {
+ // unsigned long flags;
+ // int count;
+ // int array[] __attribute__((counted_by(count)));
+ // }
+ //
+ // 1) bdos of the flexible array itself:
+ //
+ // __builtin_dynamic_object_size(p->array, 1) ==
+ // p->count * sizeof(*p->array)
+ //
+ // 2) bdos of the whole struct, including the flexible array:
+ //
+ // __builtin_dynamic_object_size(p, 1) ==
+ // sizeof(*p) + p->count * sizeof(*p->array)
+ //
+ if (const ValueDecl *CountedByFD = FindCountedByField(E)) {
+ // Find the flexible array member.
+ const RecordDecl *OuterRD =
+ CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
+ const ValueDecl *FAM = FindFlexibleArrayMemberField(getContext(),
+ OuterRD);
+
+ // Get the size of the flexible array member's base type.
+ const auto *ArrayTy = getContext().getAsArrayType(FAM->getType());
unsigned Size = getContext().getTypeSize(ArrayTy->getElementType());
- llvm::Value *CountField =
- EmitAnyExprToTemp(MemberExpr::CreateImplicit(
- getContext(), const_cast<Expr *>(ME->getBase()),
- ME->isArrow(), FD, FD->getType(), VK_LValue,
- OK_Ordinary))
- .getScalarVal();
+ // Find the outer struct expr (i.e. p in p->a.b.c.d).
+ Expr *CountedByExpr = BuildCountedByFieldExpr(const_cast<Expr *>(E),
+ CountedByFD);
+
+ llvm::Value *CountedByInstr =
+ EmitAnyExprToTemp(CountedByExpr).getScalarVal();
- llvm::Value *Mul = Builder.CreateMul(
- CountField, llvm::ConstantInt::get(CountField->getType(), Size / 8));
- Mul = Builder.CreateZExtOrTrunc(Mul, ResType);
+ llvm::Constant *ArraySize =
+ llvm::ConstantInt::get(CountedByInstr->getType(), Size / 8);
- if (ObjectSize)
- return Builder.CreateAdd(ObjectSize, Mul);
+ llvm::Value *ObjectSize = Builder.CreateMul(CountedByInstr, ArraySize);
+ ObjectSize = Builder.CreateZExtOrTrunc(ObjectSize, ResType);
+
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreImpCasts())) {
+ // The whole struct is specificed in the __bdos.
+ QualType StructTy = DRE->getType()->getPointeeType();
+ llvm::Value *StructSize = ConstantInt::get(
+ ResType, getContext().getTypeSize(StructTy) / 8, true);
+ ObjectSize = Builder.CreateAdd(StructSize, ObjectSize);
+ }
- return Mul;
+ // PULL THE STRING!!
+ return ObjectSize;
}
}
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 54a1d300a9ac738..2b39194e18ed861 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -944,14 +944,10 @@ static llvm::Value *getArrayIndexingBound(CodeGenFunction &CGF,
// Ignore pass_object_size here. It's not applicable on decayed pointers.
}
- if (FieldDecl *FD = CGF.FindCountedByField(Base, StrictFlexArraysLevel)) {
- const auto *ME = dyn_cast<MemberExpr>(CE->getSubExpr());
+ if (const ValueDecl *VD = CGF.FindCountedByField(Base)) {
IndexedType = Base->getType();
- return CGF
- .EmitAnyExprToTemp(MemberExpr::CreateImplicit(
- CGF.getContext(), const_cast<Expr *>(ME->getBase()),
- ME->isArrow(), FD, FD->getType(), VK_LValue, OK_Ordinary))
- .getScalarVal();
+ Expr *E = CGF.BuildCountedByFieldExpr(const_cast<Expr *>(Base), VD);
+ return CGF.EmitAnyExprToTemp(E).getScalarVal();
}
}
@@ -966,9 +962,68 @@ static llvm::Value *getArrayIndexingBound(CodeGenFunction &CGF,
return nullptr;
}
-FieldDecl *CodeGenFunction::FindCountedByField(
- const Expr *Base,
- LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel) {
+Expr *CodeGenFunction::BuildCountedByFieldExpr(Expr *Base,
+ const ValueDecl *CountedByVD) {
+ // Find the outer struct expr (i.e. p in p->a.b.c.d).
+ Base = Base->IgnoreImpCasts();
+ Base = Base->IgnoreParenNoopCasts(getContext());
+
+ // Work our way up the expression until we reach the DeclRefExpr.
+ while (!isa<DeclRefExpr>(Base))
+ if (auto *ME = dyn_cast<MemberExpr>(Base->IgnoreImpCasts())) {
+ Base = ME->getBase()->IgnoreImpCasts();
+ Base = Base->IgnoreParenNoopCasts(getContext());
+ }
+
+ // Add back an implicit cast to create the required pr-value.
+ Base = ImplicitCastExpr::Create(
+ getContext(), Base->getType(), CK_LValueToRValue, Base,
+ nullptr, VK_PRValue, FPOptionsOverride());
+
+ Expr *CountedByExpr = Base;
+
+ if (const auto *IFD = dyn_cast<IndirectFieldDecl>(CountedByVD)) {
+ // The counted_by field is inside an anonymous struct / union. The
+ // IndirectFieldDecl has the correct order of FieldDecls to build this
+ // easily. (Yay!)
+ for (NamedDecl *ND : IFD->chain()) {
+ ValueDecl *VD = cast<ValueDecl>(ND);
+ CountedByExpr = MemberExpr::CreateImplicit(
+ getContext(), CountedByExpr,
+ CountedByExpr->getType()->isPointerType(), VD, VD->getType(),
+ VK_LValue, OK_Ordinary);
+ }
+ } else {
+ CountedByExpr = MemberExpr::CreateImplicit(
+ getContext(), CountedByExpr,
+ CountedByExpr->getType()->isPointerType(),
+ const_cast<ValueDecl *>(CountedByVD), CountedByVD->getType(),
+ VK_LValue, OK_Ordinary);
+ }
+
+ return CountedByExpr;
+}
+
+const ValueDecl *CodeGenFunction::FindFlexibleArrayMemberField(
+ ASTContext &Ctx, const RecordDecl *RD) {
+ LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
+ getLangOpts().getStrictFlexArraysLevel();
+
+ for (const Decl *D : RD->decls()) {
+ if (const ValueDecl *VD = dyn_cast<ValueDecl>(D);
+ VD && Decl::isFlexibleArrayMemberLike(Ctx, VD, VD->getType(),
+ StrictFlexArraysLevel, true))
+ return VD;
+
+ if (const auto *Record = dyn_cast<RecordDecl>(D))
+ if (const ValueDecl *VD = FindFlexibleArrayMemberField(Ctx, Record))
+ return VD;
+ }
+
+ return nullptr;
+}
+
+const ValueDecl *CodeGenFunction::FindCountedByField(const Expr *Base) {
const ValueDecl *VD = nullptr;
Base = Base->IgnoreParenImpCasts();
@@ -984,12 +1039,14 @@ FieldDecl *CodeGenFunction::FindCountedByField(
Ty = Ty->getPointeeType();
if (const auto *RD = Ty->getAsRecordDecl())
- VD = RD->getLastField();
+ VD = FindFlexibleArrayMemberField(getContext(), RD);
} else if (const auto *CE = dyn_cast<CastExpr>(Base)) {
if (const auto *ME = dyn_cast<MemberExpr>(CE->getSubExpr()))
VD = dyn_cast<ValueDecl>(ME->getMemberDecl());
}
+ LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
+ getLangOpts().getStrictFlexArraysLevel();
const auto *FD = dyn_cast_if_present<FieldDecl>(VD);
if (!FD || !FD->getParent() ||
!Decl::isFlexibleArrayMemberLike(getContext(), FD, FD->getType(),
@@ -1000,12 +1057,14 @@ FieldDecl *CodeGenFunction::FindCountedByField(
if (!CBA)
return nullptr;
- StringRef FieldName = CBA->getCountedByField()->getName();
- auto It =
- llvm::find_if(FD->getParent()->fields(), [&](const FieldDecl *Field) {
- return FieldName == Field->getName();
- });
- return It != FD->getParent()->field_end() ? *It : nullptr;
+ const RecordDecl *RD = FD->getDeclContext()->getOuterLexicalRecordContext();
+ DeclarationName DName(CBA->getCountedByField());
+ DeclContext::lookup_result Lookup = RD->lookup(DName);
+
+ if (Lookup.empty())
+ return nullptr;
+
+ return dyn_cast<ValueDecl>(Lookup.front());
}
void CodeGenFunction::EmitBoundsCheck(const Expr *E, const Expr *Base,
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index e82115e2d706cf1..64f192037ec8ce5 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -3022,11 +3022,17 @@ class CodeGenFunction : public CodeGenTypeCache {
void EmitBoundsCheck(const Expr *E, const Expr *Base, llvm::Value *Index,
QualType IndexType, bool Accessed);
+ // Find a struct's flexible array member. It may be embedded inside multiple
+ // sub-structs, but must still be the last field.
+ const ValueDecl *FindFlexibleArrayMemberField(ASTContext &Ctx,
+ const RecordDecl *RD);
+
/// Find the FieldDecl specified in a FAM's "counted_by" attribute. Returns
/// \p nullptr if either the attribute or the field doesn't exist.
- FieldDecl *FindCountedByField(
- const Expr *Base,
- LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel);
+ const ValueDecl *FindCountedByField(const Expr *Base);
+
+ /// Build an expression accessing the "counted_by" field.
+ Expr *BuildCountedByFieldExpr(Expr *Base, const ValueDecl *CountedByVD);
llvm::Value *EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
bool isInc, bool isPre);
|
|
To answer @nickdesaulniers 's question, the testcases are in the works. :-) |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
|
|
||
| // Find the outer struct expr (i.e. p in p->a.b.c.d). | ||
| Expr *CountedByExpr = | ||
| BuildCountedByFieldExpr(const_cast<Expr *>(E), CountedByFD); |
There was a problem hiding this comment.
The amount of const_cast in handling this attribute is making me uneasy.
I guess I should have caught the first time around that this is literally building a new Expr for the member access, rather than building the GEPs in IR that I guess I would have expected for clang's Codegen to perform.
Is there any way to cut down the number of const_casts this code is doing?
There was a problem hiding this comment.
I agree that it's gross.
The const_cast s are needed, because EmitAnyExprToTemp requires it to be non-const. It's gross, but unfortunately necessary. I put the cast here to keep the BuildCountedByFieldExpr nicer with regard to const_casts. To be honest, I'm not sure why the Emit*Expr methods use non-const Expr pointers. Seems unnecessary to me... :-/
There was a problem hiding this comment.
Small correction. It's the MemberExpr::CreateImplicit c'tor that requires a non-const Expr, not the emit instructions.
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
| // 2) bdos of the whole struct, including the flexible array: | ||
| // | ||
| // __builtin_dynamic_object_size(p, 1) == | ||
| // sizeof(*p) + p->count * sizeof(*p->array) |
There was a problem hiding this comment.
Due to the alignment and padding, the size of struct s isn't necessarily the start offset of the flexible array member. FYI https://godbolt.org/z/bMb19n9Tz
I think the equation should be something like this:
max (offset(struct s, array) + p->count * sizeof(*p->array), sizeof(*p))
There was a problem hiding this comment.
I'm not sure I understand the issue involved here. From what I can tell, the getContext().getTypeSize(StructTy) returns the number of bits in the whole structure, which I divide by 8 to get bytes.
There was a problem hiding this comment.
This is about the semantics.
int array[__counted_by(count)] means you have count elements starting right from the flexible member array offset, which is offsetof(struct s, array).
Whereas, sizeof(*p) + p->count * sizeof(*p->array) indicates you have count elements starting from at the end of the struct, which isn't always the same as the offset of array because of the padding and alignment rule. https://godbolt.org/z/bMb19n9Tz shows when sizeof(struct s) and offsetof(struct s, array) aren't the same. The problem is that this can result in the object size bigger than what it should be.
There was a problem hiding this comment.
But I never want offsetof here. The sizeof(*p) will report the correct size of the array without adding on any size for the flexible array member, when it has no value (size that is). So sizeof(*p) for the array in the example is 16, which is correct. Possibly the only issue may be when the flexible array member isn't strict, i.e. it has something like int array[0] or int array[1].
There was a problem hiding this comment.
That was why I suggested to have max here: max (offset(struct s, array) + p->count * sizeof(*p->array), sizeof(*p)) so that it can return the correct size when the struct size is bigger than the end of the array due to the padding.
There was a problem hiding this comment.
Sorry, I don’t understand what we don’t mutually understand. In the same situation:
#include <string.h>
struct flex {
double dummy;
char c;
char fam[__counted_by(c)];
};
struct flex f = { 123.0, 2, { 1, 2 } };
int main() {
memset(&f, 0, sizeof(f) + f.c * sizeof(*f.fam));
}
We have an ASAN trap. Here are all the ways I can think of to reconcile this fact with the model that you propose:
- 2 is the wrong count value for a 2-element array;
- Clang is under-allocating for
f; - this is in-bounds and ASAN has a false positive;
- it's actually OK to write out of bounds when relying on
__builtin_dynamic_object_size(p, 1); - the definition chosen for
__builtin_dynamic_object_sizeis wrong.
The only one that makes sense to me is that the definition chosen for __builtin_dynamic_object_size. Which one is it to you? Do you see another way out?
There was a problem hiding this comment.
Okay, I think we are talking past to each other a little bit. That comment I was responding to this:
Why wouldn't it include the FAM part if the full struct pointer is specified to __bdos?
I'm saying "the full struct size" isn't exactly struct + fam because fam doesn't always exactly start from sizeof(struct) when there is a padding in the struct due to the alignment. And sizeof(*p) + p->count * sizeof(*p->array) gives us value that's more than the full struct size.
As we all see in the previous examples, the full struct size is offsetof(struct s, fam) + sizeof(*p->array) * p->count with the result being round up to alignof(struct s).
There was a problem hiding this comment.
@rapidsna Ah! Okay. That makes more sense, thank you. I'll see what I can do here.
There was a problem hiding this comment.
Note that it's okay for the count * sizeof(fam) to be less than the actual size of the fam. It's there to ensure the index doesn't go over what's expected.
There was a problem hiding this comment.
Okay, I added dc8f0df, which should take care of this issue here. Sorry about my confusion. PTAL.
…d of sizeof, which could emit an erroneous answer when alignment and padding are taken into account.
|
@kees, @nickdesaulniers, @rapidsna, and @apple-fcloutier Should this feature support a |
Yes. Supporting it similar to how const-sized arrays are currently handled makes sense to me. |
@bwendling It could be tracked as a separate issue, but there seems to be some inconsistencies in where bdos is derived from. For |
I think the first two Which seems correct to me, given the incorrect value in |
Oh I see.
I just wanted to call out that this case is interesting because the size derived from malloc and __counted_by compete each other. And __counted_by always wins. This conforms to how Similarly, this will also mean that , when we support |
Great! :-) As for the updating the docs and adding the intrinsic, I'd like to do that after GCC settles on their implementation. So I'd do it as a follow-up to this. |
|
@rapidsna Friendly ping. :-) |
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
| return Builder.CreateAdd(ObjectSize, Mul); | ||
| // Get the size of the flexible array member's base type. | ||
| const ValueDecl *FAM = FindFlexibleArrayMemberField(Ctx, OuterRD); | ||
| const ArrayType *ArrayTy = Ctx.getAsArrayType(FAM->getType()); |
There was a problem hiding this comment.
Should we handle the case where FAM == nullptr?
There was a problem hiding this comment.
At this point, we know that FAM can't be null. The FindCountedByField call above can only return non-null if there's a FAM.
There was a problem hiding this comment.
I see. So sounds like this relies on the fact that counted_by can currently be added to a flexible array member only? Should we add an assertion here to make it easier to deal with when that assumption breaks in future?
There was a problem hiding this comment.
There's a sema error check to ensure that it's applied only to a FAM. I suppose it can't hurt to add an assert though.
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
| llvm::Constant *ElemSize = | ||
| llvm::ConstantInt::get(CountedByInst->getType(), Size.getQuantity()); | ||
|
|
||
| llvm::Value *FAMSize = Builder.CreateMul(CountedByInst, ElemSize); |
There was a problem hiding this comment.
What happens if i > count so CountedByInst (count - i) becomes negative? Or in general, what should__bdos return if &a[i] is pointing to outside the array bounds (either it's fam or just an array)? Is it defined?
There was a problem hiding this comment.
That's more a sanitizer check, which should abort. This code is just dealing with the __bdos calls. If it returns a negative value, then that's badness. I know that it can return -1 (if the second argument is 1, otherwise 0) if it can't determine the size of the object. I don't know if it's completely invalid, per se. For instance, if you have this:
struct s {
char count;
int fam[];
};
And you want >128 elements in fam, then count is going to resemble a negative number. I know this is contrived, but it's a possibility.
There was a problem hiding this comment.
Hmm...I think maybe I should put a check in to see if the types are signed and then perform the appropriate [SU]Ext operations... Thoughts?
There was a problem hiding this comment.
Okay. I converted the instructions to carry the signedness of the counted_by field's type. PTAL.
There was a problem hiding this comment.
Hmm, it seems like __bdos returns 0 when it's pointing to an OOB object. https://godbolt.org/z/eT4c7aPWr
At least from what I'm seeing here. WDYT?
There was a problem hiding this comment.
I think you need to use pointers here. See https://godbolt.org/z/qWjs4TeW6
There was a problem hiding this comment.
However, yes it seems like I'll have to ensure that we're not pointing outside of the struct here.
|
@rapidsna My recent commits try to address a lot of the issues you brought up. If the FAM's array index is negative or out of bounds, it should now catch it and return an appropriate value. There may still be some corner cases that have to be hammered out, but I'd like to get this in if you feel it's ready, as I think the corner cases will occur infrequently. |
|
This change introduces a crash with struct irq_data {
struct irq_domain *domain;
} irq_domain_fix_revmap_d;
struct irq_domain {
struct irq_domain *parent;
int revmap_size;
struct irq_data *revmap[] __attribute__((__counted_by__(revmap_size)));
};
long irq_domain_fix_revmap_d_0;
int irq_domain_pop_irq() {
irq_domain_fix_revmap_d.domain->revmap[irq_domain_fix_revmap_d_0] = 0;
return 0;
} |
|
@nathanchance Could you try the patch in https://github.com/bwendling/llvm-project/tree/counted-by-offsets to see if it helps? I'll take a look, but it's going to compile slowly on my machine... |
|
Erm...I meant this one: main...bwendling:llvm-project:llvm-project/counted-by-offsets |
Break down the counted_by calculations so that they correctly handle anonymous structs, which are specified internally as IndirectFieldDecls. Improves the calculation of __bdos on a different field member in the struct. And also improves support for __bdos in an index into the FAM. If the index is further out than the length of the FAM, then we return __bdos's "can't determine the size" value (zero or negative one, depending on type). Also simplify the code to use helper methods to get the field referenced by counted_by and the flexible array member itself, which also had some issues with FAMs in sub-structs.
There are many issues that popped up with the counted_by feature. The patch has grown too large and approval is blocking Linux testing. Includes reverts of: commit 769bc11 ("[Clang] Implement the 'counted_by' attribute (llvm#68750)") commit bc09ec6 ("[CodeGen] Revamp counted_by calculations (llvm#70606)") commit 1a09cfb ("[Clang] counted_by attr can apply only to C99 flexible array members (llvm#72347)") commit a76adfb ("[NFC][Clang] Refactor code to calculate flexible array member size (llvm#72790)") commit d8447c7 ("[Clang] Correct handling of negative and out-of-bounds indices (llvm#71877)") Partial commit b31cd07 ("[Clang] Regenerate test checks (NFC)")
There are many issues that popped up with the counted_by feature. The patch #73730 has grown too large and approval is blocking Linux testing. Includes reverts of: commit 769bc11 ("[Clang] Implement the 'counted_by' attribute (#68750)") commit bc09ec6 ("[CodeGen] Revamp counted_by calculations (#70606)") commit 1a09cfb ("[Clang] counted_by attr can apply only to C99 flexible array members (#72347)") commit a76adfb ("[NFC][Clang] Refactor code to calculate flexible array member size (#72790)") commit d8447c7 ("[Clang] Correct handling of negative and out-of-bounds indices (#71877)") Partial commit b31cd07 ("[Clang] Regenerate test checks (NFC)") Closes #73168 Closes #75173
Break down the counted_by calculations so that they correctly handle
anonymous structs, which are specified internally as IndirectFieldDecls.
Improves the calculation of __bdos on a different field member in the struct.
And also improves support for __bdos in an index into the FAM. If the index
is further out than the length of the FAM, then we return __bdos's "can't
determine the size" value (zero or negative one, depending on type).
Also simplify the code to use helper methods to get the field referenced
by counted_by and the flexible array member itself, which also had some
issues with FAMs in sub-structs.