Hello John.
I attached reworked patch.
I couldn't found in CXXRecordDecl methods like "isFinal", but I know
only 2 ways to make class final in C++:
1. Make dtor private (will work only with dynamic allocation):
class Final {
~Final() {}
public:
Final* create() { return new Final(); }
};
2. Make dtor and ctor private, and create friend-child using virtual
inharitance:
class Finalizer {
Final() {}
~Final() {}
friend class Final;
};
class Final : virtual public Finalizer {
};
As I found, second way in clang doesn't use EmitAggregateCopy for
assignment "Final = Final", so I wrote the check for first case only.
P.S.:
Anyway I have implemented check for second case. I just excluded it from
current patch.
-Stepan.
John McCall wrote:
I think this patch is the wrong approach. Instead, EmitAggregateCopy should be
modified so that it always copies structs using their data size if either the
source or the destination might be a base-class subobject. There is no need to
adjust the alignment.
We should conservatively assume that an object might be a base-class subobject
if this is C++ and the class is not marked final. This is probably going to
hurt the performance of generated code in some cases, and we should add a flag
indicating that both objects are known to not be base-class subobjects. This
flag should default to false.
If you wouldn't mind writing up a new patch based on this approach, I'll save
the code-style comments for that.
John.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Index: lib/CodeGen/CGExprAgg.cpp
===================================================================
--- lib/CodeGen/CGExprAgg.cpp (revision 153430)
+++ lib/CodeGen/CGExprAgg.cpp (working copy)
@@ -1190,9 +1190,22 @@
return LV;
}
+static bool isFinalClass(const CXXRecordDecl *Decl) {
+ if (Decl->hasUserProvidedDefaultConstructor())
+ for (CXXRecordDecl::ctor_iterator c = Decl->ctor_begin(),
+ e = Decl->ctor_end(); c != e; ++c)
+ if (c->isDefaulted() && c->getAccess() == AS_private)
+ return true;
+
+ return Decl->hasUserDeclaredDestructor() &&
+ Decl->getDestructor()->getAccess() == AS_private;
+}
+
void CodeGenFunction::EmitAggregateCopy(llvm::Value *DestPtr,
llvm::Value *SrcPtr, QualType Ty,
- bool isVolatile, unsigned Alignment) {
+ bool isVolatile, unsigned Alignment,
+ bool checkStructsAndClasses
+ ) {
assert(!Ty->isAnyComplexType() && "Shouldn't happen for complex");
if (getContext().getLangOpts().CPlusPlus) {
@@ -1224,6 +1237,48 @@
// Get size and alignment info for this aggregate.
std::pair<CharUnits, CharUnits> TypeInfo =
getContext().getTypeInfoInChars(Ty);
+
+ // PR12204 fix: static_cast to Base class overrides the Derived class field.
+ // Consider next example:
+ //
+ // class Foo {
+ // int i;
+ // short j;
+ // };
+ //
+ // class Bar : public Foo {
+ // short t;
+ // public:
+ // void set(int v) { t = v; }
+ // int get() { return t; }
+ // };
+ // int main() {
+ // Bar a,b;
+ // b.set(7);
+ // a.set(0);
+ // static_cast<Foo&>(a) = static_cast<Foo&>(b);
+ // printf("%i\n", a.get());
+ // return 0;
+ // }
+ //
+ // Here assignment will replaced with memcpy.
+ // In usual way Foo size will rounded to 8,
+ // and memcpy(&a, &b, 8) will invoked.
+ // Due to avoid unwanted changes in Bar we should copy only bytes defined
+ // in struct/class (DataSize bytes).
+ if (checkStructsAndClasses && getContext().getLangOpts().CPlusPlus) {
+ if (const RecordType *RT = Ty->getAs<RecordType>()) {
+ CXXRecordDecl *BaseClassDecl = cast<CXXRecordDecl>(RT->getDecl());
+ if (!isFinalClass(BaseClassDecl)) {
+ const ASTRecordLayout &Layout =
+ getContext().getASTRecordLayout(RT->getDecl());
+ CharUnits DataSize = Layout.getDataSize();
+ CharUnits Size = Layout.getSize();
+ TypeInfo.first = DataSize;
+ TypeInfo.second = CharUnits::One();
+ }
+ }
+ }
if (!Alignment)
Alignment = TypeInfo.second.getQuantity();
Index: lib/CodeGen/CGExprCXX.cpp
===================================================================
--- lib/CodeGen/CGExprCXX.cpp (revision 153430)
+++ lib/CodeGen/CGExprCXX.cpp (working copy)
@@ -212,7 +212,7 @@
// We don't like to generate the trivial copy/move assignment operator
// when it isn't necessary; just produce the proper effect here.
llvm::Value *RHS = EmitLValue(*CE->arg_begin()).getAddress();
- EmitAggregateCopy(This, RHS, CE->getType());
+ EmitAggregateCopy(This, RHS, CE->getType(), false, 0, true);
return RValue::get(This);
}
@@ -339,7 +339,7 @@
MD->isTrivial()) {
llvm::Value *Src = EmitLValue(E->getArg(1)).getAddress();
QualType Ty = E->getType();
- EmitAggregateCopy(This, Src, Ty);
+ EmitAggregateCopy(This, Src, Ty, false, 0, true);
return RValue::get(This);
}
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h (revision 153430)
+++ lib/CodeGen/CodeGenFunction.h (working copy)
@@ -1643,7 +1643,8 @@
/// volatile.
void EmitAggregateCopy(llvm::Value *DestPtr, llvm::Value *SrcPtr,
QualType EltTy, bool isVolatile=false,
- unsigned Alignment = 0);
+ unsigned Alignment = 0,
+ bool checkStructsAndClasses = false);
/// StartBlock - Start new block named N. If insert block is a dummy block
/// then reuse it.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
