[Clang] Disable use of the counted_by attribute for whole struct pointers #112636
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ters
The whole struct is specificed in the __bdos. The calculation of the
whole size of the structure can be done in two ways:
1) sizeof(struct S) + count * sizeof(typeof(fam))
2) offsetof(struct S, fam) + count * sizeof(typeof(fam))
The first will add any remaining whitespace that might exist after
allocation while the second method is more precise, but not quite
expected from programmers. See [1] for a discussion of the topic.
GCC isn't (currently) able to calculate __bdos on a pointer to the whole
structure. Therefore, because of the above issue, we'll choose to match
what GCC does for consistency's sake.
[1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ for a
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Bill Wendling (bwendling) ChangesThe whole struct is specificed in the __bdos. The calculation of the whole size of the structure can be done in two ways: The first will add any remaining whitespace that might exist after allocation while the second method is more precise, but not quite expected from programmers. See [1] for a discussion of the topic. GCC isn't (currently) able to calculate __bdos on a pointer to the whole structure. Therefore, because of the above issue, we'll choose to match what GCC does for consistency's sake. [1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ for a Patch is 54.41 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112636.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index f6d7db2c204c12..362d9182e026f8 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -1013,6 +1013,24 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
// Can't find the field referenced by the "counted_by" attribute.
return nullptr;
+ if (isa<DeclRefExpr>(Base))
+ // The whole struct is specificed in the __bdos. The calculation of the
+ // whole size of the structure can be done in two ways:
+ //
+ // 1) sizeof(struct S) + count * sizeof(typeof(fam))
+ // 2) offsetof(struct S, fam) + count * sizeof(typeof(fam))
+ //
+ // The first will add any remaining whitespace that might exist after
+ // allocation while the second method is more precise, but not quite
+ // expected from programmers. See
+ // https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ for a
+ // discussion of the topic.
+ //
+ // GCC isn't (currently) able to calculate __bdos on a pointer to the whole
+ // structure. Therefore, because of the above issue, we'll choose to match
+ // what GCC does for consistency's sake.
+ return nullptr;
+
// Build a load of the counted_by field.
bool IsSigned = CountedByFD->getType()->isSignedIntegerType();
Value *CountedByInst = EmitLoadOfCountedByField(Base, FAMDecl, CountedByFD);
@@ -1043,32 +1061,9 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
CharUnits Size = Ctx.getTypeSizeInChars(ArrayTy->getElementType());
llvm::Constant *ElemSize =
llvm::ConstantInt::get(ResType, Size.getQuantity(), IsSigned);
- Value *FAMSize =
+ Value *Res =
Builder.CreateMul(CountedByInst, ElemSize, "", !IsSigned, IsSigned);
- FAMSize = Builder.CreateIntCast(FAMSize, ResType, IsSigned);
- Value *Res = FAMSize;
-
- if (isa<DeclRefExpr>(Base)) {
- // The whole struct is specificed in the __bdos.
- const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
-
- // Get the offset of the FAM.
- llvm::Constant *FAMOffset = ConstantInt::get(ResType, Offset, IsSigned);
- Value *OffsetAndFAMSize =
- Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);
-
- // Get the full size of the struct.
- llvm::Constant *SizeofStruct =
- ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);
-
- // max(sizeof(struct s),
- // offsetof(struct s, array) + p->count * sizeof(*p->array))
- Res = IsSigned
- ? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
- OffsetAndFAMSize, SizeofStruct)
- : Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
- OffsetAndFAMSize, SizeofStruct);
- }
+ Res = Builder.CreateIntCast(Res, ResType, IsSigned);
// A negative \p IdxInst or \p CountedByInst means that the index lands
// outside of the flexible array member. If that's the case, we want to
diff --git a/clang/test/CodeGen/attr-counted-by.c b/clang/test/CodeGen/attr-counted-by.c
index 4a130c5e3d401f..f70e552bca26ab 100644
--- a/clang/test/CodeGen/attr-counted-by.c
+++ b/clang/test/CodeGen/attr-counted-by.c
@@ -66,7 +66,7 @@ struct anon_struct {
// SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = icmp ult i64 [[IDXPROM]], [[TMP0]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP1]], label [[CONT3:%.*]], label [[HANDLER_OUT_OF_BOUNDS:%.*]], !prof [[PROF3:![0-9]+]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB1:[0-9]+]], i64 [[IDXPROM]]) #[[ATTR10:[0-9]+]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB1:[0-9]+]], i64 [[IDXPROM]]) #[[ATTR9:[0-9]+]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont3:
// SANITIZE-WITH-ATTR-NEXT: [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 12
@@ -114,7 +114,7 @@ void test1(struct annotated *p, int index, int val) {
// SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = icmp ult i64 [[INDEX]], [[TMP0]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP1]], label [[CONT3:%.*]], label [[HANDLER_OUT_OF_BOUNDS:%.*]], !prof [[PROF3]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB3:[0-9]+]], i64 [[INDEX]]) #[[ATTR10]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB3:[0-9]+]], i64 [[INDEX]]) #[[ATTR9]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont3:
// SANITIZE-WITH-ATTR-NEXT: [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 12
@@ -197,42 +197,26 @@ size_t test2_bdos(struct annotated *p) {
// SANITIZE-WITH-ATTR-LABEL: define dso_local void @test3(
// SANITIZE-WITH-ATTR-SAME: ptr noundef [[P:%.*]], i64 noundef [[INDEX:%.*]]) local_unnamed_addr #[[ATTR0]] {
// SANITIZE-WITH-ATTR-NEXT: entry:
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
-// SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = zext i32 [[DOT_COUNTED_BY_LOAD]] to i64, !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: [[DOTCOUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
+// SANITIZE-WITH-ATTR-NEXT: [[DOTCOUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOTCOUNTED_BY_GEP]], align 4
+// SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = zext i32 [[DOTCOUNTED_BY_LOAD]] to i64, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = icmp ult i64 [[INDEX]], [[TMP0]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP1]], label [[CONT3:%.*]], label [[HANDLER_OUT_OF_BOUNDS:%.*]], !prof [[PROF3]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB4:[0-9]+]], i64 [[INDEX]]) #[[ATTR10]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB4:[0-9]+]], i64 [[INDEX]]) #[[ATTR9]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont3:
// SANITIZE-WITH-ATTR-NEXT: [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 12
// SANITIZE-WITH-ATTR-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds nuw [0 x i32], ptr [[ARRAY]], i64 0, i64 [[INDEX]]
-// SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
-// SANITIZE-WITH-ATTR-NEXT: [[TMP3:%.*]] = shl nsw i64 [[TMP2]], 2
-// SANITIZE-WITH-ATTR-NEXT: [[TMP4:%.*]] = tail call i64 @llvm.smax.i64(i64 [[TMP3]], i64 4)
-// SANITIZE-WITH-ATTR-NEXT: [[TMP5:%.*]] = trunc i64 [[TMP4]] to i32
-// SANITIZE-WITH-ATTR-NEXT: [[TMP6:%.*]] = add i32 [[TMP5]], 12
-// SANITIZE-WITH-ATTR-NEXT: [[DOTINV:%.*]] = icmp slt i32 [[DOT_COUNTED_BY_LOAD]], 0
-// SANITIZE-WITH-ATTR-NEXT: [[CONV:%.*]] = select i1 [[DOTINV]], i32 0, i32 [[TMP6]]
-// SANITIZE-WITH-ATTR-NEXT: store i32 [[CONV]], ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA4]]
+// SANITIZE-WITH-ATTR-NEXT: store i32 -1, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA4]]
// SANITIZE-WITH-ATTR-NEXT: ret void
//
// NO-SANITIZE-WITH-ATTR-LABEL: define dso_local void @test3(
-// NO-SANITIZE-WITH-ATTR-SAME: ptr nocapture noundef [[P:%.*]], i64 noundef [[INDEX:%.*]]) local_unnamed_addr #[[ATTR1]] {
+// NO-SANITIZE-WITH-ATTR-SAME: ptr noundef [[P:%.*]], i64 noundef [[INDEX:%.*]]) local_unnamed_addr #[[ATTR0]] {
// NO-SANITIZE-WITH-ATTR-NEXT: entry:
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = shl nsw i64 [[TMP0]], 2
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = tail call i64 @llvm.smax.i64(i64 [[TMP1]], i64 4)
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP3:%.*]] = trunc i64 [[TMP2]] to i32
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP4:%.*]] = add i32 [[TMP3]], 12
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOTINV:%.*]] = icmp slt i32 [[DOT_COUNTED_BY_LOAD]], 0
-// NO-SANITIZE-WITH-ATTR-NEXT: [[CONV:%.*]] = select i1 [[DOTINV]], i32 0, i32 [[TMP4]]
// NO-SANITIZE-WITH-ATTR-NEXT: [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 12
// NO-SANITIZE-WITH-ATTR-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds nuw [0 x i32], ptr [[ARRAY]], i64 0, i64 [[INDEX]]
-// NO-SANITIZE-WITH-ATTR-NEXT: store i32 [[CONV]], ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA2]]
+// NO-SANITIZE-WITH-ATTR-NEXT: store i32 -1, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA2]]
// NO-SANITIZE-WITH-ATTR-NEXT: ret void
//
// SANITIZE-WITHOUT-ATTR-LABEL: define dso_local void @test3(
@@ -254,34 +238,18 @@ size_t test2_bdos(struct annotated *p) {
void test3(struct annotated *p, size_t index) {
// This test differs from 'test2' by checking bdos on the whole array and not
// just the FAM.
- p->array[index] = __builtin_dynamic_object_size(p, 1);
+ p->array[index] = __builtin_dynamic_object_size(p, 0);
}
-// SANITIZE-WITH-ATTR-LABEL: define dso_local range(i64 0, 8589934601) i64 @test3_bdos(
-// SANITIZE-WITH-ATTR-SAME: ptr nocapture noundef readonly [[P:%.*]]) local_unnamed_addr #[[ATTR2]] {
+// SANITIZE-WITH-ATTR-LABEL: define dso_local i64 @test3_bdos(
+// SANITIZE-WITH-ATTR-SAME: ptr noundef readnone [[P:%.*]]) local_unnamed_addr #[[ATTR3:[0-9]+]] {
// SANITIZE-WITH-ATTR-NEXT: entry:
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
-// SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
-// SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = shl nsw i64 [[TMP0]], 2
-// SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = tail call i64 @llvm.smax.i64(i64 [[TMP1]], i64 4)
-// SANITIZE-WITH-ATTR-NEXT: [[TMP3:%.*]] = add nuw nsw i64 [[TMP2]], 12
-// SANITIZE-WITH-ATTR-NEXT: [[TMP4:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], -1
-// SANITIZE-WITH-ATTR-NEXT: [[TMP5:%.*]] = select i1 [[TMP4]], i64 [[TMP3]], i64 0
-// SANITIZE-WITH-ATTR-NEXT: ret i64 [[TMP5]]
+// SANITIZE-WITH-ATTR-NEXT: ret i64 -1
//
-// NO-SANITIZE-WITH-ATTR-LABEL: define dso_local range(i64 0, 8589934601) i64 @test3_bdos(
-// NO-SANITIZE-WITH-ATTR-SAME: ptr nocapture noundef readonly [[P:%.*]]) local_unnamed_addr #[[ATTR2]] {
+// NO-SANITIZE-WITH-ATTR-LABEL: define dso_local i64 @test3_bdos(
+// NO-SANITIZE-WITH-ATTR-SAME: ptr noundef readnone [[P:%.*]]) local_unnamed_addr #[[ATTR3:[0-9]+]] {
// NO-SANITIZE-WITH-ATTR-NEXT: entry:
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = shl nsw i64 [[TMP0]], 2
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = tail call i64 @llvm.smax.i64(i64 [[TMP1]], i64 4)
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP3:%.*]] = add nuw nsw i64 [[TMP2]], 12
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP4:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], -1
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP5:%.*]] = select i1 [[TMP4]], i64 [[TMP3]], i64 0
-// NO-SANITIZE-WITH-ATTR-NEXT: ret i64 [[TMP5]]
+// NO-SANITIZE-WITH-ATTR-NEXT: ret i64 -1
//
// SANITIZE-WITHOUT-ATTR-LABEL: define dso_local i64 @test3_bdos(
// SANITIZE-WITHOUT-ATTR-SAME: ptr noundef readnone [[P:%.*]]) local_unnamed_addr #[[ATTR2:[0-9]+]] {
@@ -294,7 +262,7 @@ void test3(struct annotated *p, size_t index) {
// NO-SANITIZE-WITHOUT-ATTR-NEXT: ret i64 -1
//
size_t test3_bdos(struct annotated *p) {
- return __builtin_dynamic_object_size(p, 1);
+ return __builtin_dynamic_object_size(p, 0);
}
// SANITIZE-WITH-ATTR-LABEL: define dso_local void @test4(
@@ -308,7 +276,7 @@ size_t test3_bdos(struct annotated *p) {
// SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = icmp ult i64 [[IDXPROM]], [[TMP0]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP1]], label [[CONT4:%.*]], label [[HANDLER_OUT_OF_BOUNDS:%.*]], !prof [[PROF3]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB5:[0-9]+]], i64 [[IDXPROM]]) #[[ATTR10]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB5:[0-9]+]], i64 [[IDXPROM]]) #[[ATTR9]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont4:
// SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], 2
@@ -325,7 +293,7 @@ size_t test3_bdos(struct annotated *p) {
// SANITIZE-WITH-ATTR-NEXT: [[TMP7:%.*]] = icmp ult i64 [[IDXPROM12]], [[TMP6]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP7]], label [[CONT19:%.*]], label [[HANDLER_OUT_OF_BOUNDS15:%.*]], !prof [[PROF3]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds15:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB6:[0-9]+]], i64 [[IDXPROM12]]) #[[ATTR10]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB6:[0-9]+]], i64 [[IDXPROM12]]) #[[ATTR9]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont19:
// SANITIZE-WITH-ATTR-NEXT: [[TMP8:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD6]], 3
@@ -342,7 +310,7 @@ size_t test3_bdos(struct annotated *p) {
// SANITIZE-WITH-ATTR-NEXT: [[TMP13:%.*]] = icmp ult i64 [[IDXPROM28]], [[TMP12]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP13]], label [[CONT35:%.*]], label [[HANDLER_OUT_OF_BOUNDS31:%.*]], !prof [[PROF3]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds31:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB7:[0-9]+]], i64 [[IDXPROM28]]) #[[ATTR10]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB7:[0-9]+]], i64 [[IDXPROM28]]) #[[ATTR9]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont35:
// SANITIZE-WITH-ATTR-NEXT: [[ARRAYIDX33:%.*]] = getelementptr inbounds [0 x i32], ptr [[ARRAY]], i64 0, i64 [[IDXPROM28]]
@@ -488,39 +456,27 @@ size_t test4_bdos(struct annotated *p, int index) {
// SANITIZE-WITH-ATTR-LABEL: define dso_local void @test5(
// SANITIZE-WITH-ATTR-SAME: ptr noundef [[P:%.*]], i32 noundef [[INDEX:%.*]]) local_unnamed_addr #[[ATTR0]] {
// SANITIZE-WITH-ATTR-NEXT: entry:
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD:%.*]] = load i64, ptr [[DOT_COUNTED_BY_GEP]], align 4
// SANITIZE-WITH-ATTR-NEXT: [[IDXPROM:%.*]] = sext i32 [[INDEX]] to i64
-// SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = icmp ugt i64 [[DOT_COUNTED_BY_LOAD]], [[IDXPROM]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: [[DOTCOUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
+// SANITIZE-WITH-ATTR-NEXT: [[DOTCOUNTED_BY_LOAD:%.*]] = load i64, ptr [[DOTCOUNTED_BY_GEP]], align 4
+// SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = icmp ugt i64 [[DOTCOUNTED_BY_LOAD]], [[IDXPROM]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: br i1 [[TMP0]], label [[CONT3:%.*]], label [[HANDLER_OUT_OF_BOUNDS:%.*]], !prof [[PROF3]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: handler.out_of_bounds:
-// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB8:[0-9]+]], i64 [[IDXPROM]]) #[[ATTR10]], !nosanitize [[META2]]
+// SANITIZE-WITH-ATTR-NEXT: tail call void @__ubsan_handle_out_of_bounds_abort(ptr nonnull @[[GLOB8:[0-9]+]], i64 [[IDXPROM]]) #[[ATTR9]], !nosanitize [[META2]]
// SANITIZE-WITH-ATTR-NEXT: unreachable, !nosanitize [[META2]]
// SANITIZE-WITH-ATTR: cont3:
// SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 16
// SANITIZE-WITH-ATTR-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [0 x i32], ptr [[TMP1]], i64 0, i64 [[IDXPROM]]
-// SANITIZE-WITH-ATTR-NEXT: [[DOTINV:%.*]] = icmp slt i64 [[DOT_COUNTED_BY_LOAD]], 0
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD_TR:%.*]] = trunc i64 [[DOT_COUNTED_BY_LOAD]] to i32
-// SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = shl i32 [[DOT_COUNTED_BY_LOAD_TR]], 2
-// SANITIZE-WITH-ATTR-NEXT: [[TMP3:%.*]] = add i32 [[TMP2]], 16
-// SANITIZE-WITH-ATTR-NEXT: [[CONV:%.*]] = select i1 [[DOTINV]], i32 0, i32 [[TMP3]]
-// SANITIZE-WITH-ATTR-NEXT: store i32 [[CONV]], ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA4]]
+// SANITIZE-WITH-ATTR-NEXT: store i32 -1, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA4]]
// SANITIZE-WITH-ATTR-NEXT: ret void
//
// NO-SANITIZE-WITH-ATTR-LABEL: define dso_local void @test5(
-// NO-SANITIZE-WITH-ATTR-SAME: ptr nocapture noundef [[P:%.*]], i32 noundef [[INDEX:%.*]]) local_unnamed_addr #[[ATTR1]] {
+// NO-SANITIZE-WITH-ATTR-SAME: ptr noundef [[P:%.*]], i32 noundef [[INDEX:%.*]]) local_unnamed_addr #[[ATTR0]] {
// NO-SANITIZE-WITH-ATTR-NEXT: entry:
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD:%.*]] = load i64, ptr [[DOT_COUNTED_BY_GEP]], align 4
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOTINV:%.*]] = icmp slt i64 [[DOT_COUNTED_BY_LOAD]], 0
-// NO-SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_LOAD_TR:%.*]] = trunc i64 [[DOT_COUNTED_BY_LOAD]] to i32
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = shl i32 [[DOT_COUNTED_BY_LOAD_TR]], 2
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP1:%.*]] = add i32 [[TMP0]], 16
-// NO-SANITIZE-WITH-ATTR-NEXT: [[CONV:%.*]] = select i1 [[DOTINV]], i32 0, i32 [[TMP1]]
-// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP2:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 16
+// NO-SANITIZE-WITH-ATTR-NEXT: [[TMP0:%.*]] = getelementptr inbounds nuw i8, ptr [[P]], i64 16
// NO-SANITIZE-WITH-ATTR-NEXT: [[IDXPROM:%.*]] = sext i32 [[INDEX]] to i64
-// NO-SANITIZE-WITH-ATTR-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [0 x i32], ptr [[TMP2]], i64 0, i64 [[IDXPROM]]
-// NO-SANITIZE-WITH-ATTR-NEXT: store i32 [[CONV]], ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA2]]
+// NO-SANITIZE-WITH-ATTR-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds [0 x i32], ptr [[TMP0]], i64 0, i64 [[IDXPROM]]
+// NO-SANITIZE-WITH-ATTR-NEXT: store i32 -1, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA2]]
// NO-SANITIZE-WITH-ATTR-NEXT: ret void
//
// SANITIZE-WITHOUT-ATTR-LABEL: define dso_local void @test5(
@@ -545,27 +501,15 @@ void test5(struct anon_struct *p, int index) {
p->array[index] = __builtin_dynamic_object_size(p, 1);
}
-// SANITIZE-WITH-ATTR-LABEL: define dso_local range(i64 16, 1) i64 @test5_bdos(
-// SANITIZE-WITH-ATTR-SAME: ptr nocapture noundef readonly [[P:%.*]]) local_unnamed_addr #[[ATTR2]] {
+// SANITIZE-WITH-ATTR-LABEL: define dso_local i64 @test5_bdos(
+// SANITIZE-WITH-ATTR-SAME: ptr noundef readnone [[P:%.*]]) local_unnamed_addr #[[ATTR3]] {
// SANITIZE-WITH-ATTR-NEXT: entry:
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 8
-// SANITIZE-WITH-ATTR-NEXT: [[DOT_COU...
[truncated]
|
efriedma-quic
approved these changes
Oct 17, 2024
| @@ -1013,6 +1013,24 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type, | |||
| // Can't find the field referenced by the "counted_by" attribute. | |||
| return nullptr; | |||
|
|
|||
| if (isa<DeclRefExpr>(Base)) | |||
There was a problem hiding this comment.
I'm not sure this reliably detects a "whole struct"? It's not clear to me why we want to treat int f(struct X*p) { return __builtin_dynamic_object_size(p, 0); } differently from int f2(struct X**p) { return __builtin_dynamic_object_size(*p, 0); } here.
Maybe this is fine if other parts of the code detect "whole struct" the same way, though.
There was a problem hiding this comment.
It's possibly a bug, but we currently don't support int f2(struct X **p) { return __builtin_dynamic_object_size(*p, 0); } and just return -1. We do, however, support something like &p->array[0], which the hope is that it's more common than the previous. From what I can tell, this builtin is kind of a "best effort", so falling back to -1 when faced with something unexpected should hopefully only result in a bug report.
Co-authored-by: Eli Friedman <[email protected]>
bwendling
added a commit
to bwendling/llvm-project
that referenced
this pull request
Oct 17, 2024
…ters (llvm#112636) The whole struct is specificed in the __bdos. The calculation of the whole size of the structure can be done in two ways: 1) sizeof(struct S) + count * sizeof(typeof(fam)) 2) offsetof(struct S, fam) + count * sizeof(typeof(fam)) The first will add any remaining whitespace that might exist after allocation while the second method is more precise, but not quite expected from programmers. See [1] for a discussion of the topic. GCC isn't (currently) able to calculate __bdos on a pointer to the whole structure. Therefore, because of the above issue, we'll choose to match what GCC does for consistency's sake. [1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ Co-authored-by: Eli Friedman <[email protected]>
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 21, 2024
…ters (llvm#112636) The whole struct is specificed in the __bdos. The calculation of the whole size of the structure can be done in two ways: 1) sizeof(struct S) + count * sizeof(typeof(fam)) 2) offsetof(struct S, fam) + count * sizeof(typeof(fam)) The first will add any remaining whitespace that might exist after allocation while the second method is more precise, but not quite expected from programmers. See [1] for a discussion of the topic. GCC isn't (currently) able to calculate __bdos on a pointer to the whole structure. Therefore, because of the above issue, we'll choose to match what GCC does for consistency's sake. [1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ Co-authored-by: Eli Friedman <[email protected]>
EricWF
pushed a commit
to efcs/llvm-project
that referenced
this pull request
Oct 22, 2024
…ters (llvm#112636) The whole struct is specificed in the __bdos. The calculation of the whole size of the structure can be done in two ways: 1) sizeof(struct S) + count * sizeof(typeof(fam)) 2) offsetof(struct S, fam) + count * sizeof(typeof(fam)) The first will add any remaining whitespace that might exist after allocation while the second method is more precise, but not quite expected from programmers. See [1] for a discussion of the topic. GCC isn't (currently) able to calculate __bdos on a pointer to the whole structure. Therefore, because of the above issue, we'll choose to match what GCC does for consistency's sake. [1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ Co-authored-by: Eli Friedman <[email protected]>
|
For the record, I think the most correct definition, in terms of "this is how much memory you should allocate for a struct with a flexible array member" is this: max(
sizeof(struct S), // always at least the size of the struct itself
round_up(
alignof(struct S), // size must be a multiple of alignment
offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0])
)
) |
I mean it would be useful to round up to the alignment for when you wanne have an array of the structs, but I'm not sure this is actually required by the standard. Do you have more justification for the alignment requirement on structs containing FAMs? I would argue the minimum "legal" amount to allocate is: max(
sizeof(struct S),
offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0])
) |
|
Also see previous discussion here: #111015 |
Here's an example: struct S {
int foo;
char fam[];
};
struct S *s = malloc(9);
s->fam[4];
which I interpret as the largest N such that Well, for N = 4 we have |
|
I think you're correct |
|
I was forgetting about the padding when thinking about this part of the quote:
|
|
@bwendling, @kees What do y'all think about that? |
|
The expression can be simplified to: round_up(
alignof(struct S)
offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0])
)As the padding at the end of the structure is always smaller than the alignof. |
|
I think the calculation in #112636 (comment) is correct. It's probably worthwhile to perform some analysis to see if using that to calculate the new size results in allocation size changes in the kernel. If not, then perhaps we could reinstate the "whole struct" code. |
This can calculation can result in both larger and smaller sizes compared to what the kernel currently does, so we can't reinstate the whole struct code until the |
|
Example of both cases: https://godbolt.org/z/dhzG69sab |
|
We do have to consider though that when llvm-project/clang/docs/LanguageExtensions.rst Lines 5502 to 5507 in 3b88805 So I think the correct result for round_up(
alignof(struct S)
offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0])
)
+ alignof(struct S) - 1because all objects that are 1 byte smaller than the calculation in #112636 (comment) plus Using that calculation for the two examples above looks to actually solve the issue: https://godbolt.org/z/Pdx7Mbano (correction: https://godbolt.org/z/TYPYGvK6G) |
Using this example, every object of sizes 8 to 11 bytes, inclusive, behaves like the struct defined with N = 4. Therefore |
|
But for now, I think we should merge #112786 into 19.1.3 so that the we can have that be the cutoff in the kernel. |
1Naim
pushed a commit
to CachyOS/linux
that referenced
this pull request
Oct 27, 2024
This patch disables __counted_by for clang versions < 19.1.3 because of the two issues listed below. It does this by introducing CONFIG_CC_HAS_COUNTED_BY. 1. clang < 19.1.2 has a bug that can lead to __bdos returning 0: llvm/llvm-project#110497 2. clang < 19.1.3 has a bug that can lead to __bdos being off by 4: llvm/llvm-project#112636 Fixes: c8248fa ("Compiler Attributes: counted_by: Adjust name and identifier expansion") Cc: [email protected] # 6.6.x: 16c31dd: Compiler Attributes: counted_by: bump min gcc version Cc: [email protected] # 6.6.x: 2993eb7: Compiler Attributes: counted_by: fixup clang URL Cc: [email protected] # 6.6.x: 231dc3f: lkdtm/bugs: Improve warning message for compilers without counted_by support Cc: [email protected] # 6.6.x Reported-by: Nathan Chancellor <[email protected]> Closes: https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/ Reported-by: kernel test robot <[email protected]> Closes: https://lore.kernel.org/oe-lkp/[email protected] Link: https://lore.kernel.org/all/Zw8iawAF5W2uzGuh@archlinux/T/#m204c09f63c076586a02d194b87dffc7e81b8de7b Signed-off-by: Jan Hendrik Farr <[email protected]>
1Naim
pushed a commit
to CachyOS/linux
that referenced
this pull request
Oct 28, 2024
This patch disables __counted_by for clang versions < 19.1.3 because of the two issues listed below. It does this by introducing CONFIG_CC_HAS_COUNTED_BY. 1. clang < 19.1.2 has a bug that can lead to __bdos returning 0: llvm/llvm-project#110497 2. clang < 19.1.3 has a bug that can lead to __bdos being off by 4: llvm/llvm-project#112636 Fixes: c8248fa ("Compiler Attributes: counted_by: Adjust name and identifier expansion") Cc: [email protected] # 6.6.x: 16c31dd: Compiler Attributes: counted_by: bump min gcc version Cc: [email protected] # 6.6.x: 2993eb7: Compiler Attributes: counted_by: fixup clang URL Cc: [email protected] # 6.6.x: 231dc3f: lkdtm/bugs: Improve warning message for compilers without counted_by support Cc: [email protected] # 6.6.x Reported-by: Nathan Chancellor <[email protected]> Closes: https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/ Reported-by: kernel test robot <[email protected]> Closes: https://lore.kernel.org/oe-lkp/[email protected] Link: https://lore.kernel.org/all/Zw8iawAF5W2uzGuh@archlinux/T/#m204c09f63c076586a02d194b87dffc7e81b8de7b Signed-off-by: Jan Hendrik Farr <[email protected]>
tru
pushed a commit
to bwendling/llvm-project
that referenced
this pull request
Oct 29, 2024
…ters (llvm#112636) The whole struct is specificed in the __bdos. The calculation of the whole size of the structure can be done in two ways: 1) sizeof(struct S) + count * sizeof(typeof(fam)) 2) offsetof(struct S, fam) + count * sizeof(typeof(fam)) The first will add any remaining whitespace that might exist after allocation while the second method is more precise, but not quite expected from programmers. See [1] for a discussion of the topic. GCC isn't (currently) able to calculate __bdos on a pointer to the whole structure. Therefore, because of the above issue, we'll choose to match what GCC does for consistency's sake. [1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/ Co-authored-by: Eli Friedman <[email protected]>
intel-lab-lkp
pushed a commit
to intel-lab-lkp/linux
that referenced
this pull request
Oct 29, 2024
This patch disables __counted_by for clang versions < 19.1.3 because of the two issues listed below. It does this by introducing CONFIG_CC_HAS_COUNTED_BY. 1. clang < 19.1.2 has a bug that can lead to __bdos returning 0: llvm/llvm-project#110497 2. clang < 19.1.3 has a bug that can lead to __bdos being off by 4: llvm/llvm-project#112636 Fixes: c8248fa ("Compiler Attributes: counted_by: Adjust name and identifier expansion") Cc: [email protected] # 6.6.x: 16c31dd: Compiler Attributes: counted_by: bump min gcc version Cc: [email protected] # 6.6.x: 2993eb7: Compiler Attributes: counted_by: fixup clang URL Cc: [email protected] # 6.6.x: 231dc3f: lkdtm/bugs: Improve warning message for compilers without counted_by support Cc: [email protected] # 6.6.x Reported-by: Nathan Chancellor <[email protected]> Closes: https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/ Reported-by: kernel test robot <[email protected]> Closes: https://lore.kernel.org/oe-lkp/[email protected] Link: https://lore.kernel.org/all/Zw8iawAF5W2uzGuh@archlinux/T/#m204c09f63c076586a02d194b87dffc7e81b8de7b Suggested-by: Nathan Chancellor <[email protected]> Signed-off-by: Jan Hendrik Farr <[email protected]> Reviewed-by: Nathan Chancellor <[email protected]> Tested-by: Nathan Chancellor <[email protected]> Reviewed-by: Miguel Ojeda <[email protected]>
I think that's right for sane ABIs. I assume Clang always lays structs out like this. My use of struct S {
int foo;
char fam[N];
};unnecessarily and you can't really predict that. Ignoring that case, your simplification seems fine to me. |
1Naim
pushed a commit
to CachyOS/linux
that referenced
this pull request
Nov 4, 2024
This patch disables __counted_by for clang versions < 19.1.3 because of the two issues listed below. It does this by introducing CONFIG_CC_HAS_COUNTED_BY. 1. clang < 19.1.2 has a bug that can lead to __bdos returning 0: llvm/llvm-project#110497 2. clang < 19.1.3 has a bug that can lead to __bdos being off by 4: llvm/llvm-project#112636 Fixes: c8248fa ("Compiler Attributes: counted_by: Adjust name and identifier expansion") Cc: [email protected] # 6.6.x: 16c31dd: Compiler Attributes: counted_by: bump min gcc version Cc: [email protected] # 6.6.x: 2993eb7: Compiler Attributes: counted_by: fixup clang URL Cc: [email protected] # 6.6.x: 231dc3f: lkdtm/bugs: Improve warning message for compilers without counted_by support Cc: [email protected] # 6.6.x Reported-by: Nathan Chancellor <[email protected]> Closes: https://lore.kernel.org/all/20240913164630.GA4091534@thelio-3990X/ Reported-by: kernel test robot <[email protected]> Closes: https://lore.kernel.org/oe-lkp/[email protected] Link: https://lore.kernel.org/all/Zw8iawAF5W2uzGuh@archlinux/T/#m204c09f63c076586a02d194b87dffc7e81b8de7b Signed-off-by: Jan Hendrik Farr <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The whole struct is specificed in the __bdos. The calculation of the whole size of the structure can be done in two ways:
The first will add any remaining whitespace that might exist after allocation while the second method is more precise, but not quite expected from programmers. See [1] for a discussion of the topic.
GCC isn't (currently) able to calculate __bdos on a pointer to the whole structure. Therefore, because of the above issue, we'll choose to match what GCC does for consistency's sake.
[1] https://lore.kernel.org/lkml/ZvV6X5FPBBW7CO1f@archlinux/