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
+797 −127
Merged

[CodeGen] Revamp counted_by calculations #70606

Show file tree
Hide file tree
Changes from 13 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
105 changes: 60 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,68 @@ 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);
// 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) ==
// max(sizeof(struct s),
// offsetof(struct s, array) + p->count * sizeof(*p->array))
//
if (const ValueDecl *CountedByFD = FindCountedByField(E)) {
const RecordDecl *OuterRD =
CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
ASTContext &Ctx = getContext();

// Load the counted_by field.
const Expr *CountedByExpr = BuildCountedByFieldExpr(E, CountedByFD);
llvm::Value *CountedByInst =
EmitAnyExprToTemp(CountedByExpr).getScalarVal();

// Get the size of the flexible array member's base type.
const ValueDecl *FAM = FindFlexibleArrayMemberField(Ctx, OuterRD);
const ArrayType *ArrayTy = Ctx.getAsArrayType(FAM->getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle the case where FAM == nullptr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

CharUnits Size = Ctx.getTypeSizeInChars(ArrayTy->getElementType());
llvm::Constant *ElemSize =
llvm::ConstantInt::get(CountedByInst->getType(), Size.getQuantity());

llvm::Value *FAMSize = Builder.CreateMul(CountedByInst, ElemSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. I converted the instructions to carry the signedness of the counted_by field's type. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you need to use pointers here. See https://godbolt.org/z/qWjs4TeW6

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, yes it seems like I'll have to ensure that we're not pointing outside of the struct here.

llvm::Value *Res = Builder.CreateZExtOrTrunc(FAMSize, ResType);

if (const auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) {
// The whole struct is specificed in the __bdos.
// Get the full size of the struct.
const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
llvm::Value *SizeofStruct =
ConstantInt::get(ResType, Layout.getSize().getQuantity(), true);

// Get the offset of the FAM.
CharUnits Offset = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FAM));
llvm::Value *FAMOffset =
ConstantInt::get(ResType, Offset.getQuantity(), true);

// max(sizeof(struct s),
// offsetof(struct s, array) + p->count * sizeof(*p->array))
llvm::Value *OffsetAndFAMSize = Builder.CreateAdd(FAMOffset, Res);
Res = Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
OffsetAndFAMSize, SizeofStruct);
}

// At this point, we know that \p ME is a flexible array member.
const auto *ArrayTy = getContext().getAsArrayType(ME->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();

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 Res;
}
}

Expand Down
90 changes: 73 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();
const Expr *E = CGF.BuildCountedByFieldExpr(Base, VD);
return CGF.EmitAnyExprToTemp(E).getScalarVal();
}
}

Expand All @@ -966,9 +962,65 @@ static llvm::Value *getArrayIndexingBound(CodeGenFunction &CGF,
return nullptr;
}

FieldDecl *CodeGenFunction::FindCountedByField(
const Expr *Base,
LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel) {
const Expr *
CodeGenFunction::BuildCountedByFieldExpr(const Expr *Base,
const ValueDecl *CountedByVD) {
// Find the outer struct expr (i.e. p in p->a.b.c.d).
Expr *CountedByExpr = const_cast<Expr *>(Base)->IgnoreParenImpCasts();

// Work our way up the expression until we reach the DeclRefExpr.
while (!isa<DeclRefExpr>(CountedByExpr))
if (const auto *ME = dyn_cast<MemberExpr>(CountedByExpr))
CountedByExpr = ME->getBase()->IgnoreParenImpCasts();

// Add back an implicit cast to create the required pr-value.
CountedByExpr = ImplicitCastExpr::Create(
getContext(), CountedByExpr->getType(), CK_LValueToRValue, CountedByExpr,
nullptr, VK_PRValue, FPOptionsOverride());

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()) {
auto *VD = cast<ValueDecl>(ND);
CountedByExpr =
MemberExpr::CreateImplicit(getContext(), CountedByExpr,
CountedByExpr->getType()->isPointerType(),
VD, VD->getType(), VK_LValue, OK_Ordinary);
}
} else {
CountedByExpr = MemberExpr::CreateImplicit(
getContext(), const_cast<Expr *>(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 auto *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 +1036,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 +1054,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
13 changes: 10 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,18 @@ 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.
const Expr *BuildCountedByFieldExpr(const Expr *Base,
const ValueDecl *CountedByVD);

llvm::Value *EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
bool isInc, bool isPre);
Expand Down
Loading