<div dir="ltr">It's a wrong-code bugfix, but we've had the bug since Clang 3.5, so it doesn't seem urgent.<div><br></div><div>Hans, what do you think? I don't think we've had any field reports of miscompiles (though someone did notice the ubsan false-positive).</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 1 Feb 2020 at 05:04, Shoaib Meenai <<a href="mailto:smeenai@fb.com">smeenai@fb.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Should this be cherry-picked to 10.0?<br>
<br>
On 1/31/20, 7:09 PM, "cfe-commits on behalf of Richard Smith via cfe-commits" <<a href="mailto:cfe-commits-bounces@lists.llvm.org" target="_blank">cfe-commits-bounces@lists.llvm.org</a> on behalf of <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
<br>
<br>
Author: Richard Smith<br>
Date: 2020-01-31T19:08:17-08:00<br>
New Revision: 0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4.diff</a><br>
<br>
LOG: Don't assume a reference refers to at least sizeof(T) bytes.<br>
<br>
When T is a class type, only nvsize(T) bytes need be accessible through<br>
the reference. We had matching bugs in the application of the<br>
dereferenceable attribute and in -fsanitize=undefined.<br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
clang/include/clang/AST/DeclCXX.h<br>
clang/lib/AST/DeclCXX.cpp<br>
clang/lib/CodeGen/CGCall.cpp<br>
clang/lib/CodeGen/CGClass.cpp<br>
clang/lib/CodeGen/CGExpr.cpp<br>
clang/lib/CodeGen/CodeGenModule.h<br>
clang/test/CodeGenCXX/catch-undef-behavior.cpp<br>
clang/test/CodeGenCXX/thunks.cpp<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h<br>
index 2e8e31dbf4c7..6d3a833b5037 100644<br>
--- a/clang/include/clang/AST/DeclCXX.h<br>
+++ b/clang/include/clang/AST/DeclCXX.h<br>
@@ -1696,6 +1696,10 @@ class CXXRecordDecl : public RecordDecl {<br>
/// actually abstract.<br>
bool mayBeAbstract() const;<br>
<br>
+ /// Determine whether it's impossible for a class to be derived from this<br>
+ /// class. This is best-effort, and may conservatively return false.<br>
+ bool isEffectivelyFinal() const;<br>
+<br>
/// If this is the closure type of a lambda expression, retrieve the<br>
/// number to be used for name mangling in the Itanium C++ ABI.<br>
///<br>
<br>
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp<br>
index 227fe80ccab4..931a1141b1b4 100644<br>
--- a/clang/lib/AST/DeclCXX.cpp<br>
+++ b/clang/lib/AST/DeclCXX.cpp<br>
@@ -1923,6 +1923,18 @@ bool CXXRecordDecl::mayBeAbstract() const {<br>
return false;<br>
}<br>
<br>
+bool CXXRecordDecl::isEffectivelyFinal() const {<br>
+ auto *Def = getDefinition();<br>
+ if (!Def)<br>
+ return false;<br>
+ if (Def->hasAttr<FinalAttr>())<br>
+ return true;<br>
+ if (const auto *Dtor = Def->getDestructor())<br>
+ if (Dtor->hasAttr<FinalAttr>())<br>
+ return true;<br>
+ return false;<br>
+}<br>
+<br>
void CXXDeductionGuideDecl::anchor() {}<br>
<br>
bool ExplicitSpecifier::isEquivalent(const ExplicitSpecifier Other) const {<br>
@@ -2142,12 +2154,8 @@ CXXMethodDecl *CXXMethodDecl::getDevirtualizedMethod(const Expr *Base,<br>
// Similarly, if the class itself or its destructor is marked 'final',<br>
// the class can't be derived from and we can therefore devirtualize the <br>
// member function call.<br>
- if (BestDynamicDecl->hasAttr<FinalAttr>())<br>
+ if (BestDynamicDecl->isEffectivelyFinal())<br>
return DevirtualizedMethod;<br>
- if (const auto *dtor = BestDynamicDecl->getDestructor()) {<br>
- if (dtor->hasAttr<FinalAttr>())<br>
- return DevirtualizedMethod;<br>
- }<br>
<br>
if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {<br>
if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))<br>
<br>
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp<br>
index 3f132a0a62aa..9ed2ccd54487 100644<br>
--- a/clang/lib/CodeGen/CGCall.cpp<br>
+++ b/clang/lib/CodeGen/CGCall.cpp<br>
@@ -2054,8 +2054,8 @@ void CodeGenModule::ConstructAttributeList(<br>
if (const auto *RefTy = RetTy->getAs<ReferenceType>()) {<br>
QualType PTy = RefTy->getPointeeType();<br>
if (!PTy->isIncompleteType() && PTy->isConstantSizeType())<br>
- RetAttrs.addDereferenceableAttr(getContext().getTypeSizeInChars(PTy)<br>
- .getQuantity());<br>
+ RetAttrs.addDereferenceableAttr(<br>
+ getMinimumObjectSize(PTy).getQuantity());<br>
else if (getContext().getTargetAddressSpace(PTy) == 0 &&<br>
!CodeGenOpts.NullPointerIsValid)<br>
RetAttrs.addAttribute(llvm::Attribute::NonNull);<br>
@@ -2164,8 +2164,8 @@ void CodeGenModule::ConstructAttributeList(<br>
if (const auto *RefTy = ParamType->getAs<ReferenceType>()) {<br>
QualType PTy = RefTy->getPointeeType();<br>
if (!PTy->isIncompleteType() && PTy->isConstantSizeType())<br>
- Attrs.addDereferenceableAttr(getContext().getTypeSizeInChars(PTy)<br>
- .getQuantity());<br>
+ Attrs.addDereferenceableAttr(<br>
+ getMinimumObjectSize(PTy).getQuantity());<br>
else if (getContext().getTargetAddressSpace(PTy) == 0 &&<br>
!CodeGenOpts.NullPointerIsValid)<br>
Attrs.addAttribute(llvm::Attribute::NonNull);<br>
<br>
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp<br>
index 7389207bc8ad..acc9a9ec4f4a 100644<br>
--- a/clang/lib/CodeGen/CGClass.cpp<br>
+++ b/clang/lib/CodeGen/CGClass.cpp<br>
@@ -35,20 +35,37 @@ using namespace CodeGen;<br>
/// Return the best known alignment for an unknown pointer to a<br>
/// particular class.<br>
CharUnits CodeGenModule::getClassPointerAlignment(const CXXRecordDecl *RD) {<br>
- if (!RD->isCompleteDefinition())<br>
+ if (!RD->hasDefinition())<br>
return CharUnits::One(); // Hopefully won't be used anywhere.<br>
<br>
auto &layout = getContext().getASTRecordLayout(RD);<br>
<br>
// If the class is final, then we know that the pointer points to an<br>
// object of that type and can use the full alignment.<br>
- if (RD->hasAttr<FinalAttr>()) {<br>
+ if (RD->isEffectivelyFinal())<br>
return layout.getAlignment();<br>
<br>
// Otherwise, we have to assume it could be a subclass.<br>
- } else {<br>
- return layout.getNonVirtualAlignment();<br>
- }<br>
+ return layout.getNonVirtualAlignment();<br>
+}<br>
+<br>
+/// Return the smallest possible amount of storage that might be allocated<br>
+/// starting from the beginning of an object of a particular class.<br>
+///<br>
+/// This may be smaller than sizeof(RD) if RD has virtual base classes.<br>
+CharUnits CodeGenModule::getMinimumClassObjectSize(const CXXRecordDecl *RD) {<br>
+ if (!RD->hasDefinition())<br>
+ return CharUnits::One();<br>
+<br>
+ auto &layout = getContext().getASTRecordLayout(RD);<br>
+<br>
+ // If the class is final, then we know that the pointer points to an<br>
+ // object of that type and can use the full alignment.<br>
+ if (RD->isEffectivelyFinal())<br>
+ return layout.getSize();<br>
+<br>
+ // Otherwise, we have to assume it could be a subclass.<br>
+ return std::max(layout.getNonVirtualSize(), CharUnits::One());<br>
}<br>
<br>
/// Return the best known alignment for a pointer to a virtual base,<br>
<br>
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp<br>
index f1a5e3dcb33c..4b8a31c180c8 100644<br>
--- a/clang/lib/CodeGen/CGExpr.cpp<br>
+++ b/clang/lib/CodeGen/CGExpr.cpp<br>
@@ -711,7 +711,7 @@ void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,<br>
if (SanOpts.has(SanitizerKind::ObjectSize) &&<br>
!SkippedChecks.has(SanitizerKind::ObjectSize) &&<br>
!Ty->isIncompleteType()) {<br>
- uint64_t TySize = getContext().getTypeSizeInChars(Ty).getQuantity();<br>
+ uint64_t TySize = CGM.getMinimumObjectSize(Ty).getQuantity();<br>
llvm::Value *Size = llvm::ConstantInt::get(IntPtrTy, TySize);<br>
if (ArraySize)<br>
Size = Builder.CreateMul(Size, ArraySize);<br>
@@ -742,7 +742,9 @@ void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,<br>
!SkippedChecks.has(SanitizerKind::Alignment)) {<br>
AlignVal = Alignment.getQuantity();<br>
if (!Ty->isIncompleteType() && !AlignVal)<br>
- AlignVal = getContext().getTypeAlignInChars(Ty).getQuantity();<br>
+ AlignVal =<br>
+ getNaturalTypeAlignment(Ty, nullptr, nullptr, /*ForPointeeType=*/true)<br>
+ .getQuantity();<br>
<br>
// The glvalue must be suitably aligned.<br>
if (AlignVal > 1 &&<br>
<br>
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h<br>
index a711d5ccba0e..e8162fcfc0cd 100644<br>
--- a/clang/lib/CodeGen/CodeGenModule.h<br>
+++ b/clang/lib/CodeGen/CodeGenModule.h<br>
@@ -868,6 +868,17 @@ class CodeGenModule : public CodeGenTypeCache {<br>
/// Returns the assumed alignment of an opaque pointer to the given class.<br>
CharUnits getClassPointerAlignment(const CXXRecordDecl *CD);<br>
<br>
+ /// Returns the minimum object size for an object of the given class type<br>
+ /// (or a class derived from it).<br>
+ CharUnits getMinimumClassObjectSize(const CXXRecordDecl *CD);<br>
+<br>
+ /// Returns the minimum object size for an object of the given type.<br>
+ CharUnits getMinimumObjectSize(QualType Ty) {<br>
+ if (CXXRecordDecl *RD = Ty->getAsCXXRecordDecl())<br>
+ return getMinimumClassObjectSize(RD);<br>
+ return getContext().getTypeSizeInChars(Ty);<br>
+ }<br>
+<br>
/// Returns the assumed alignment of a virtual base of a class.<br>
CharUnits getVBaseAlignment(CharUnits DerivedAlign,<br>
const CXXRecordDecl *Derived,<br>
<br>
diff --git a/clang/test/CodeGenCXX/catch-undef-behavior.cpp b/clang/test/CodeGenCXX/catch-undef-behavior.cpp<br>
index ba72af038aba..c5aec53ad724 100644<br>
--- a/clang/test/CodeGenCXX/catch-undef-behavior.cpp<br>
+++ b/clang/test/CodeGenCXX/catch-undef-behavior.cpp<br>
@@ -426,6 +426,25 @@ void indirect_function_call(void (*p)(int)) {<br>
p(42);<br>
}<br>
<br>
+namespace VBaseObjectSize {<br>
+ // Note: C is laid out such that offsetof(C, B) + sizeof(B) extends outside<br>
+ // the C object.<br>
+ struct alignas(16) A { void *a1, *a2; };<br>
+ struct B : virtual A { void *b; };<br>
+ struct C : virtual A, virtual B {};<br>
+ // CHECK-LABEL: define {{.*}} @_ZN15VBaseObjectSize1fERNS_1BE(<br>
+ B &f(B &b) {<br>
+ // Size check: check for nvsize(B) == 16 (do not require size(B) == 32)<br>
+ // CHECK: [[SIZE:%.+]] = call i{{32|64}} @llvm.objectsize.i64.p0i8(<br>
+ // CHECK: icmp uge i{{32|64}} [[SIZE]], 16,<br>
+<br>
+ // Alignment check: check for nvalign(B) == 8 (do not require align(B) == 16)<br>
+ // CHECK: [[PTRTOINT:%.+]] = ptrtoint {{.*}} to i64,<br>
+ // CHECK: and i64 [[PTRTOINT]], 7,<br>
+ return b;<br>
+ }<br>
+}<br>
+<br>
namespace FunctionSanitizerVirtualCalls {<br>
struct A {<br>
virtual void f() {}<br>
<br>
diff --git a/clang/test/CodeGenCXX/thunks.cpp b/clang/test/CodeGenCXX/thunks.cpp<br>
index 4a0610ed488d..fe7d656eb7e5 100644<br>
--- a/clang/test/CodeGenCXX/thunks.cpp<br>
+++ b/clang/test/CodeGenCXX/thunks.cpp<br>
@@ -412,7 +412,7 @@ namespace Test13 {<br>
// CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 8<br>
// CHECK: ret %"struct.Test13::D"*<br>
<br>
- // WIN64-LABEL: define weak_odr dso_local dereferenceable(32) %"struct.Test13::D"* @"?foo1@D@Test13@@$4PPPPPPPE@A@EAAAEAUB1@2@XZ"(<br>
+ // WIN64-LABEL: define weak_odr dso_local dereferenceable(8) %"struct.Test13::D"* @"?foo1@D@Test13@@$4PPPPPPPE@A@EAAAEAUB1@2@XZ"(<br>
// This adjustment.<br>
// WIN64: getelementptr inbounds i8, i8* {{.*}}, i64 -12<br>
// Call implementation.<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=CfI0YzHTWSKGGaKoCCW_TcBbgs9qek2vZSS_cdHZDIw&s=sCuW5vldFWSuM_t_J3Hbv_-sZKRs7NYXs2ihx__jJ9A&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=CfI0YzHTWSKGGaKoCCW_TcBbgs9qek2vZSS_cdHZDIw&s=sCuW5vldFWSuM_t_J3Hbv_-sZKRs7NYXs2ihx__jJ9A&e=</a> <br>
<br>
<br>
</blockquote></div>