Skip to content

[CodeGen] Revamp counted_by calculations #70606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Nov 9, 2023
Merged

[CodeGen] Revamp counted_by calculations #70606

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
19dd7db
[CodeGen] Revamp counted_by calculations
bwendling Oct 29, 2023
36b5271
Reformat with clang-format
bwendling Oct 29, 2023
8c6880b
Update testcase with anonymous structs and a union
bwendling Oct 30, 2023
a5fac56
Use 'IgnoreParenImpCasts' instead of just 'IgnoreImpCasts'
bwendling Oct 30, 2023
4d2e39c
Use 'IgnoreParenImpCasts' and adjust code to use fewer 'Expr' variables.
bwendling Oct 30, 2023
e70b487
Reduce number of const_casts.
bwendling Oct 30, 2023
3f98e8e
Use 'getTypeSizeInChars' instead of dividing by 8
bwendling Oct 31, 2023
b503de0
Use 'getTypeSizeInChars' instead of dividing by 8
bwendling Oct 31, 2023
82588c3
Format
bwendling Oct 31, 2023
dc8f0df
Use 'offsetof(struct s, array) + p->count * sizeof(*p->array)' instea…
bwendling Oct 31, 2023
6260945
Adjust testcase to reflect 'offsetof' change.
bwendling Nov 1, 2023
7a20a34
Return the max of the struct size or offset + FAM size.
bwendling Nov 1, 2023
1e94fec
Small reordering of code and added more comments.
bwendling Nov 1, 2023
c9819d7
Support a __bdos() on an index into the FAM.
bwendling Nov 1, 2023
163de1d
Merge branch 'llvm:main' into counted-by-expansion
bwendling Nov 6, 2023
7abab68
Ignore parens for '&(p->fam[10])' expressions and use signed extend i…
bwendling Nov 8, 2023
cbdfc3b
Update testcase
bwendling Nov 8, 2023
c15f340
Pass the signedness throughout the IR construction.
bwendling Nov 8, 2023
e6f5299
Start at the outer-most lexical RecordDecl instead of assuming the FA…
bwendling Nov 9, 2023
ff321b0
If the count is negative, we've gone past the end of the FAM. Return 0.
bwendling Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 52 additions & 45 deletions clang/lib/CodeGen/CGBuiltin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@rapidsna rapidsna Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Collaborator Author

@bwendling bwendling Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rapidsna rapidsna Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@bwendling bwendling Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Contributor

@rapidsna rapidsna Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@apple-fcloutier apple-fcloutier Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_size is 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?

Copy link
Contributor

@rapidsna rapidsna Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

@bwendling bwendling Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rapidsna Ah! Okay. That makes more sense, thank you. I'll see what I can do here.

Copy link
Collaborator Author

@bwendling bwendling Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@bwendling bwendling Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I added dc8f0df, which should take care of this issue here. Sorry about my confusion. PTAL.

//
if (const ValueDecl *CountedByFD = FindCountedByField(E)) {
// Find the flexible array member.
const RecordDecl *OuterRD =
CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
const ValueDecl *FAM =
FindFlexibleArrayMemberField(getContext(), OuterRD);

// Find the outer struct expr (i.e. p in p->a.b.c.d).
Expr *CountedByExpr =
BuildCountedByFieldExpr(const_cast<Expr *>(E), CountedByFD);
Copy link
Member

@nickdesaulniers nickdesaulniers Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@bwendling bwendling Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... :-/

Copy link
Collaborator Author

@bwendling bwendling Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small correction. It's the MemberExpr::CreateImplicit c'tor that requires a non-const Expr, not the emit instructions.


// Load the counted_by field.
llvm::Value *CountedByInstr =
EmitAnyExprToTemp(CountedByExpr).getScalarVal();

// 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::Constant *ArraySize =
llvm::ConstantInt::get(CountedByInstr->getType(), Size / 8);

llvm::Value *ObjectSize = Builder.CreateMul(CountedByInstr, ArraySize);
ObjectSize = Builder.CreateZExtOrTrunc(ObjectSize, ResType);

if (const auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) {
// 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);
}

llvm::Value *CountField =
EmitAnyExprToTemp(MemberExpr::CreateImplicit(
getContext(), const_cast<Expr *>(ME->getBase()),
ME->isArrow(), FD, FD->getType(), VK_LValue,
OK_Ordinary))
.getScalarVal();

llvm::Value *Mul = Builder.CreateMul(
CountField, llvm::ConstantInt::get(CountField->getType(), Size / 8));
Mul = Builder.CreateZExtOrTrunc(Mul, ResType);

if (ObjectSize)
return Builder.CreateAdd(ObjectSize, Mul);

return Mul;
// PULL THE STRING!!
return ObjectSize;
}
}

Expand Down
93 changes: 76 additions & 17 deletions clang/lib/CodeGen/CGExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand All @@ -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();
Expand All @@ -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(),
Expand All @@ -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,
Expand Down
12 changes: 9 additions & 3 deletions clang/lib/CodeGen/CodeGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading