[PATCH] D104097: [analyzer] Fix calculating offset for fields with an empty type
This revision was automatically updated to reflect the committed changes. Closed by commit rGc558b1fca735: [analyzer] Fix calculating offset for fields with an empty type (authored by jubnzv). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104097/new/ https://reviews.llvm.org/D104097 Files: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp clang/test/Analysis/padding_no_unique_address.cpp Index: clang/test/Analysis/padding_no_unique_address.cpp === --- /dev/null +++ clang/test/Analysis/padding_no_unique_address.cpp @@ -0,0 +1,30 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-linux-gnu -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s + +class Empty {}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn1' (6 padding}} +struct NoUniqueAddressWarn1 { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn2' (6 padding}} +struct NoUniqueAddressWarn2 { +char c1; +[[no_unique_address]] Empty e1, e2; +int i; +char c2; +}; + +struct NoUniqueAddressNoWarn1 { + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; + +struct NoUniqueAddressNoWarn2 { + char c1; + [[no_unique_address]] Empty e1, e2; +}; Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment()) Index: clang/test/Analysis/padding_no_unique_address.cpp === --- /dev/null +++ clang/test/Analysis/padding_no_unique_address.cpp @@ -0,0 +1,30 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-linux-gnu -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s + +class Empty {}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn1' (6 padding}} +struct NoUniqueAddressWarn1 { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn2' (6 padding}} +struct NoUniqueAddressWarn2 { +char c1; +[[no_unique_address]] Empty e1, e2; +int i; +char c2; +}; + +struct NoUniqueAddressNoWarn1 { + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; + +struct NoUniqueAddressNoWarn2 { + char c1; + [[no_unique_address]] Empty e1, e2; +}; Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment())
[PATCH] D104097: [analyzer] Fix calculating offset for fields with an empty type
jubnzv updated this revision to Diff 352361. jubnzv added a comment. Added two tests that check whether we still calculate padding correctly for structs with `[[no_unique_address]]` on two consecutive fields CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104097/new/ https://reviews.llvm.org/D104097 Files: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp clang/test/Analysis/padding_no_unique_address.cpp Index: clang/test/Analysis/padding_no_unique_address.cpp === --- /dev/null +++ clang/test/Analysis/padding_no_unique_address.cpp @@ -0,0 +1,30 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-linux-gnu -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s + +class Empty {}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn1' (6 padding}} +struct NoUniqueAddressWarn1 { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn2' (6 padding}} +struct NoUniqueAddressWarn2 { +char c1; +[[no_unique_address]] Empty e1, e2; +int i; +char c2; +}; + +struct NoUniqueAddressNoWarn1 { + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; + +struct NoUniqueAddressNoWarn2 { + char c1; + [[no_unique_address]] Empty e1, e2; +}; Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment()) Index: clang/test/Analysis/padding_no_unique_address.cpp === --- /dev/null +++ clang/test/Analysis/padding_no_unique_address.cpp @@ -0,0 +1,30 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-linux-gnu -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s + +class Empty {}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn1' (6 padding}} +struct NoUniqueAddressWarn1 { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn2' (6 padding}} +struct NoUniqueAddressWarn2 { +char c1; +[[no_unique_address]] Empty e1, e2; +int i; +char c2; +}; + +struct NoUniqueAddressNoWarn1 { + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; + +struct NoUniqueAddressNoWarn2 { + char c1; + [[no_unique_address]] Empty e1, e2; +}; Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment())
[PATCH] D104097: [analyzer] Fix calculating offset for fields with an empty type
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. This looks correct, thanks! I think there are some weird rules with respect to `[[no_unique_address]]` on two consecutive fields when there's only one other field in the structure, eg. struct S { char c; [[no_unique_address]] Empty e1, e2; }; will have a size of two bytes according to https://en.cppreference.com/w/cpp/language/attributes/no_unique_address. It's probably worth a test case to see if we still calculate padding correctly in such cases but I don't insist. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104097/new/ https://reviews.llvm.org/D104097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D104097: [analyzer] Fix calculating offset for fields with an empty type
jubnzv updated this revision to Diff 351423. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104097/new/ https://reviews.llvm.org/D104097 Files: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp clang/test/Analysis/padding_no_unique_address.cpp Index: clang/test/Analysis/padding_no_unique_address.cpp === --- /dev/null +++ clang/test/Analysis/padding_no_unique_address.cpp @@ -0,0 +1,18 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-linux-gnu -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s + +class Empty {}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn' (6 padding}} +struct NoUniqueAddressWarn { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +struct NoUniqueAddressNoWarn { // no-warning + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; + Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment()) Index: clang/test/Analysis/padding_no_unique_address.cpp === --- /dev/null +++ clang/test/Analysis/padding_no_unique_address.cpp @@ -0,0 +1,18 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-linux-gnu -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s + +class Empty {}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn' (6 padding}} +struct NoUniqueAddressWarn { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +struct NoUniqueAddressNoWarn { // no-warning + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; + Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment()) ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D104097: [analyzer] Fix calculating offset for fields with an empty type
jubnzv created this revision. jubnzv added reviewers: steakhal, NoQ, vsavchenko, Szelethus, xazax.hun, balazske. jubnzv added a project: clang. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. jubnzv requested review of this revision. Fix offset calculation routines in padding checker to avoid assertion errors described in the following issue: https://bugs.llvm.org/show_bug.cgi?id=50426. The fields that are subojbects of zero size, marked with [[no_unique_address]] or empty bitfields will be excluded from the padding calculation routines. https://reviews.llvm.org/D104097 Files: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp clang/test/Analysis/padding_cpp.cpp Index: clang/test/Analysis/padding_cpp.cpp === --- clang/test/Analysis/padding_cpp.cpp +++ clang/test/Analysis/padding_cpp.cpp @@ -200,3 +200,17 @@ // expected-warning@+1{{Excessive padding in 'class (lambda}} auto lambda1 = [ c1 = G.c1, i = G.i, c2 = G.c2 ]{}; auto lambda2 = [ i = G.i, c1 = G.c1, c2 = G.c2 ]{}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn' (6 padding}} +struct NoUniqueAddressWarn { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +struct NoUniqueAddressNoWarn { // no-warning + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment()) Index: clang/test/Analysis/padding_cpp.cpp === --- clang/test/Analysis/padding_cpp.cpp +++ clang/test/Analysis/padding_cpp.cpp @@ -200,3 +200,17 @@ // expected-warning@+1{{Excessive padding in 'class (lambda}} auto lambda1 = [ c1 = G.c1, i = G.i, c2 = G.c2 ]{}; auto lambda2 = [ i = G.i, c1 = G.c1, c2 = G.c2 ]{}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct NoUniqueAddressWarn' (6 padding}} +struct NoUniqueAddressWarn { + char c1; + [[no_unique_address]] Empty empty; + int i; + char c2; +}; + +struct NoUniqueAddressNoWarn { // no-warning + char c1; + [[no_unique_address]] Empty empty; + char c2; +}; Index: clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp === --- clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -193,6 +193,11 @@ CharUnits PaddingSum; CharUnits Offset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); for (const FieldDecl *FD : RD->fields()) { + // Skip field that is a subobject of zero size, marked with + // [[no_unique_address]] or an empty bitfield, because its address can be + // set the same as the other fields addresses. + if (FD->isZeroSize(ASTContext)) +continue; // This checker only cares about the padded size of the // field, and not the data size. If the field is a record // with tail padding, then we won't put that number in our @@ -249,7 +254,7 @@ RetVal.Field = FD; auto = FD->getASTContext(); auto Info = Ctx.getTypeInfoInChars(FD->getType()); - RetVal.Size = Info.Width; + RetVal.Size = FD->isZeroSize(Ctx) ? CharUnits::Zero() : Info.Width; RetVal.Align = Info.Align; assert(llvm::isPowerOf2_64(RetVal.Align.getQuantity())); if (auto Max = FD->getMaxAlignment()) ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits