From: Fariborz Jahanian Date: Fri, 25 Jan 2013 23:57:05 +0000 (+0000) Subject: patch for PR9027 and // rdar://11861085 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=3ac83d69c61238cd0d38e90fcdd03390530ab2fb;p=clang patch for PR9027 and // rdar://11861085 Title: [PR9027] volatile struct bug: member is not loaded at -O; This is caused by last flag passed to @llvm.memcpy being false, not honoring that aggregate has at least one 'volatile' data member (even though aggregate itself has not been qualified as 'volatile'. As a result, optimization optimizes away the memcpy altogether. Patch review by John MaCall (I still need to fix up a test though). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@173535 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index 8b9688baea..cfcbeb2c00 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -2930,6 +2930,10 @@ class RecordDecl : public TagDecl { /// HasObjectMember - This is true if this struct has at least one member /// containing an Objective-C object pointer type. bool HasObjectMember : 1; + + /// HasVolatileMember - This is true if struct has at least one member of + /// 'volatile' type. + bool HasVolatileMember : 1; /// \brief Whether the field declarations of this record have been loaded /// from external storage. To avoid unnecessary deserialization of @@ -2986,6 +2990,9 @@ public: bool hasObjectMember() const { return HasObjectMember; } void setHasObjectMember (bool val) { HasObjectMember = val; } + bool hasVolatileMember() const { return HasVolatileMember; } + void setHasVolatileMember (bool val) { HasVolatileMember = val; } + /// \brief Determines whether this declaration represents the /// injected class name. /// diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 6ecc2cce53..f9ad946109 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -2734,6 +2734,7 @@ RecordDecl::RecordDecl(Kind DK, TagKind TK, DeclContext *DC, HasFlexibleArrayMember = false; AnonymousStructOrUnion = false; HasObjectMember = false; + HasVolatileMember = false; LoadedFieldsFromExternalStorage = false; assert(classof(static_cast(this)) && "Invalid Kind!"); } diff --git a/lib/AST/DeclCXX.cpp b/lib/AST/DeclCXX.cpp index 87574d39e9..90a2b4ab46 100644 --- a/lib/AST/DeclCXX.cpp +++ b/lib/AST/DeclCXX.cpp @@ -301,6 +301,9 @@ CXXRecordDecl::setBases(CXXBaseSpecifier const * const *Bases, // has an Objective-C object member. if (BaseClassDecl->hasObjectMember()) setHasObjectMember(true); + + if (BaseClassDecl->hasVolatileMember()) + setHasVolatileMember(true); // Keep track of the presence of mutable fields. if (BaseClassDecl->hasMutableFields()) @@ -751,6 +754,8 @@ void CXXRecordDecl::addedMember(Decl *D) { data().HasIrrelevantDestructor = false; if (FieldRec->hasObjectMember()) setHasObjectMember(true); + if (FieldRec->hasVolatileMember()) + setHasVolatileMember(true); // C++0x [class]p7: // A standard-layout class is a class that: diff --git a/lib/CodeGen/CGExprAgg.cpp b/lib/CodeGen/CGExprAgg.cpp index a35ef0c931..8c64e8a6e7 100644 --- a/lib/CodeGen/CGExprAgg.cpp +++ b/lib/CodeGen/CGExprAgg.cpp @@ -792,6 +792,11 @@ void AggExprEmitter::VisitBinAssign(const BinaryOperator *E) { AggValueSlot::forLValue(LHS, AggValueSlot::IsDestructed, needsGC(E->getLHS()->getType()), AggValueSlot::IsAliased); + // A non-volatile aggregate destination might have volatile member. + if (!LHSSlot.isVolatile() && + CGF.hasVolatileMember(E->getLHS()->getType())) + LHSSlot.setVolatile(true); + CGF.EmitAggExpr(E->getRHS(), LHSSlot); // Copy into the destination if the assignment isn't ignored. diff --git a/lib/CodeGen/CGValue.h b/lib/CodeGen/CGValue.h index 7a2b8e30b1..01dee1f628 100644 --- a/lib/CodeGen/CGValue.h +++ b/lib/CodeGen/CGValue.h @@ -412,6 +412,10 @@ public: return Quals.hasVolatile(); } + void setVolatile(bool flag) { + Quals.setVolatile(flag); + } + Qualifiers::ObjCLifetime getObjCLifetime() const { return Quals.getObjCLifetime(); } diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index afe99381ae..bfcc77a3fd 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -1665,14 +1665,24 @@ public: void EmitExprAsInit(const Expr *init, const ValueDecl *D, LValue lvalue, bool capturedByInit); + /// hasVolatileMember - returns true if aggregate type has a volatile + /// member. + bool hasVolatileMember(QualType T) { + if (const RecordType *RT = T->getAs()) { + const RecordDecl *RD = cast(RT->getDecl()); + return RD->hasVolatileMember(); + } + return false; + } /// EmitAggregateCopy - Emit an aggrate assignment. /// /// The difference to EmitAggregateCopy is that tail padding is not copied. /// This is required for correctness when assigning non-POD structures in C++. void EmitAggregateAssign(llvm::Value *DestPtr, llvm::Value *SrcPtr, - QualType EltTy, bool isVolatile=false, - CharUnits Alignment = CharUnits::Zero()) { - EmitAggregateCopy(DestPtr, SrcPtr, EltTy, isVolatile, Alignment, true); + QualType EltTy) { + bool IsVolatile = hasVolatileMember(EltTy); + EmitAggregateCopy(DestPtr, SrcPtr, EltTy, IsVolatile, CharUnits::Zero(), + true); } /// EmitAggregateCopy - Emit an aggrate copy. diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index d584ed8571..136d12ebc3 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -10484,6 +10484,8 @@ void Sema::ActOnFields(Scope* S, } if (Record && FDTTy->getDecl()->hasObjectMember()) Record->setHasObjectMember(true); + if (Record && FDTTy->getDecl()->hasVolatileMember()) + Record->setHasVolatileMember(true); } else if (FDTy->isObjCObjectType()) { /// A field cannot be an Objective-c object Diag(FD->getLocation(), diag::err_statically_allocated_object) @@ -10530,6 +10532,8 @@ void Sema::ActOnFields(Scope* S, } } } + if (Record && FD->getType().isVolatileQualified()) + Record->setHasVolatileMember(true); // Keep track of the number of named members. if (FD->getIdentifier()) ++NumNamedMembers; diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 4ae67a04e7..5fe1011579 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -475,6 +475,7 @@ void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) { RD->setHasFlexibleArrayMember(Record[Idx++]); RD->setAnonymousStructOrUnion(Record[Idx++]); RD->setHasObjectMember(Record[Idx++]); + RD->setHasVolatileMember(Record[Idx++]); } void ASTDeclReader::VisitValueDecl(ValueDecl *VD) { diff --git a/lib/Serialization/ASTWriterDecl.cpp b/lib/Serialization/ASTWriterDecl.cpp index cd4eb310f0..7be49a283f 100644 --- a/lib/Serialization/ASTWriterDecl.cpp +++ b/lib/Serialization/ASTWriterDecl.cpp @@ -263,6 +263,7 @@ void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) { Record.push_back(D->hasFlexibleArrayMember()); Record.push_back(D->isAnonymousStructOrUnion()); Record.push_back(D->hasObjectMember()); + Record.push_back(D->hasVolatileMember()); if (!D->hasAttrs() && !D->isImplicit() && @@ -1456,6 +1457,7 @@ void ASTWriter::WriteDeclsBlockAbbrevs() { Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // FlexibleArrayMember Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // AnonymousStructUnion Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // hasObjectMember + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // hasVolatileMember // DC Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // LexicalOffset Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // VisibleOffset diff --git a/test/CodeGen/no-opt-volatile-memcpy.c b/test/CodeGen/no-opt-volatile-memcpy.c new file mode 100644 index 0000000000..0fab363280 --- /dev/null +++ b/test/CodeGen/no-opt-volatile-memcpy.c @@ -0,0 +1,40 @@ +// RUN: %clang_cc1 -O0 -triple=x86_64-apple-darwin -emit-llvm -o - %s | FileCheck %s +// rdar://11861085 + +struct s { + char filler [128]; + volatile int x; +}; + +struct s gs; + +void foo (void) { + struct s ls; + ls = ls; + gs = gs; + ls = gs; +} +// CHECK: define void @foo() +// CHECK: %[[LS:.*]] = alloca %struct.s, align 4 +// CHECK-NEXT: %[[ZERO:.*]] = bitcast %struct.s* %[[LS]] to i8* +// CHECK-NEXT: %[[ONE:.*]] = bitcast %struct.s* %[[LS]] to i8* +// CHECK-NEXT: call void @llvm.memcpy.{{.*}}(i8* %[[ZERO]], i8* %[[ONE]], i64 132, i32 4, i1 true) +// CHECK-NEXT: call void @llvm.memcpy.{{.*}}(i8* getelementptr inbounds (%struct.s* @gs, i32 0, i32 0, i32 0), i8* getelementptr inbounds (%struct.s* @gs, i32 0, i32 0, i32 0), i64 132, i32 4, i1 true) +// CHECK-NEXT: %[[TWO:.*]] = bitcast %struct.s* %[[LS]] to i8* +// CHECK-NEXT: call void @llvm.memcpy.{{.*}}(i8* %[[TWO]], i8* getelementptr inbounds (%struct.s* @gs, i32 0, i32 0, i32 0), i64 132, i32 4, i1 true) + + +struct s1 { + struct s y; +}; + +struct s1 s; + +void fee (void) { + s = s; + s.y = gs; +} +// CHECK: define void @fee() +// CHECK: call void @llvm.memcpy.{{.*}}(i8* getelementptr inbounds (%struct.s1* @s, i32 0, i32 0, i32 0, i32 0), i8* getelementptr inbounds (%struct.s1* @s, i32 0, i32 0, i32 0, i32 0), i64 132, i32 4, i1 true) +// CHECK-NEXT: call void @llvm.memcpy.{{.*}}(i8* getelementptr inbounds (%struct.s1* @s, i32 0, i32 0, i32 0, i32 0), i8* getelementptr inbounds (%struct.s* @gs, i32 0, i32 0, i32 0), i64 132, i32 4, i1 true) + diff --git a/test/CodeGenCXX/no-opt-volatile-memcpy.cpp b/test/CodeGenCXX/no-opt-volatile-memcpy.cpp new file mode 100644 index 0000000000..a0008a7704 --- /dev/null +++ b/test/CodeGenCXX/no-opt-volatile-memcpy.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 -O -triple=x86_64-apple-darwin -emit-llvm -o - %s | FileCheck %s +// rdar://11861085 + +struct s { + char filler [128]; + volatile int x; +}; + +struct s gs; + +void foo (void) { + struct s ls; + ls = ls; + gs = gs; + ls = gs; +} +// CHECK: call void @llvm.memcpy +// CHECK: call void @llvm.memcpy +// CHECK: call void @llvm.memcpy + +struct s1 { + struct s y; +}; + +struct s1 s; + +void fee (void) { + s = s; + s.y = gs; +} +// CHECK: call void @llvm.memcpy +// CHECK: call void @llvm.memcpy + + +struct d : s1 { +}; + +d gd; + +void gorf(void) { + gd = gd; +} +// CHECK: call void @llvm.memcpy +