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 8b9688baead8bc83d74e9a0f86ae087c4fbc7cb8..cfcbeb2c005aa5e1b3fccbf1d04fb3060af6f577 100644 (file)
--- 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 6ecc2cce53cb89cec0ae007c9c7e072bb0a71a24..f9ad9461095294a3106ff2ea1b1fb7592455e342 100644 (file)
--- 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<Decl*>(this)) && "Invalid Kind!");
 }

diff --git a/lib/AST/DeclCXX.cpp b/lib/AST/DeclCXX.cpp
index 87574d39e9d169af39c4ef98e5964a680b3db67a..90a2b4ab4691109bc399a50201dc18350fa8993d 100644 (file)
--- 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 a35ef0c9318a894cf962e0b4566fbd7dd880f889..8c64e8a6e7264d1f1204d896d4e2eb46e71c6b16 100644 (file)
--- 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 7a2b8e30b1ac657895fc85e72eb8ebf5002dc429..01dee1f6283a349215fb8eaa991a04e04ac00dd0 100644 (file)
--- 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 afe99381ae3a89463c36a1e6bc9786b059f688f4..bfcc77a3fdb56473e050d8bbacdad54a616cb334 100644 (file)
--- 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<RecordType>()) {
+      const RecordDecl *RD = cast<RecordDecl>(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 d584ed85710e225d9b5852feaefde950a28f382f..136d12ebc317531943ee038d4c9b9025d0233031 100644 (file)
--- 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 4ae67a04e78151a8640122fee289e38853f4beef..5fe1011579a776659cc48238b896f6fdb8530b54 100644 (file)
--- 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 cd4eb310f09e7e42d38db10ee7be47ef022ca899..7be49a283f835e242c959011ea35435c541c5367 100644 (file)
--- 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 (file)
index 0000000..0fab363
--- /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 (file)
index 0000000..a0008a7
--- /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
+