[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[Warren Ristow via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
wristow marked an inline comment as done.
Closed by commit rL293911: Prevent ICE in dllexport class with _Atomic data 
member (authored by wristow).

Changed prior to commit:
  https://reviews.llvm.org/D29208?vs=86767&id=86848#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29208

Files:
  cfe/trunk/lib/CodeGen/CGClass.cpp
  cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp


Index: cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp
===
--- cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp
+++ cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++11 
-fms-extensions -O0 -o - %s | FileCheck --check-prefix=M32 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++11 
-fms-extensions -O0 -o - %s | FileCheck --check-prefix=M64 %s
+
+struct __declspec(dllexport) SomeStruct {
+  // Copy assignment operator should be produced, and exported:
+  // M32: define weak_odr dllexport x86_thiscallcc dereferenceable({{[0-9]+}}) 
%struct.SomeStruct* @"\01??4SomeStruct@@QAEAAU0@ABU0@@Z"
+  // M64: define weak_odr dllexportdereferenceable({{[0-9]+}}) 
%struct.SomeStruct* @"\01??4SomeStruct@@QEAAAEAU0@AEBU0@@Z"
+  _Atomic(int) mData;
+};
Index: cfe/trunk/lib/CodeGen/CGClass.cpp
===
--- cfe/trunk/lib/CodeGen/CGClass.cpp
+++ cfe/trunk/lib/CodeGen/CGClass.cpp
@@ -1131,10 +1131,11 @@
   RHS = EC->getSubExpr();
 if (!RHS)
   return nullptr;
-MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
-  return nullptr;
-return Field;
+if (MemberExpr *ME2 = dyn_cast(RHS)) {
+  if (ME2->getMemberDecl() == Field)
+return Field;
+}
+return nullptr;
   } else if (CXXMemberCallExpr *MCE = dyn_cast(S)) {
 CXXMethodDecl *MD = dyn_cast(MCE->getCalleeDecl());
 if (!(MD && isMemcpyEquivalentSpecialMember(MD)))


Index: cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp
===
--- cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp
+++ cfe/trunk/test/CodeGenCXX/atomic-dllexport.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++11 -fms-extensions -O0 -o - %s | FileCheck --check-prefix=M32 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++11 -fms-extensions -O0 -o - %s | FileCheck --check-prefix=M64 %s
+
+struct __declspec(dllexport) SomeStruct {
+  // Copy assignment operator should be produced, and exported:
+  // M32: define weak_odr dllexport x86_thiscallcc dereferenceable({{[0-9]+}}) %struct.SomeStruct* @"\01??4SomeStruct@@QAEAAU0@ABU0@@Z"
+  // M64: define weak_odr dllexportdereferenceable({{[0-9]+}}) %struct.SomeStruct* @"\01??4SomeStruct@@QEAAAEAU0@AEBU0@@Z"
+  _Atomic(int) mData;
+};
Index: cfe/trunk/lib/CodeGen/CGClass.cpp
===
--- cfe/trunk/lib/CodeGen/CGClass.cpp
+++ cfe/trunk/lib/CodeGen/CGClass.cpp
@@ -1131,10 +1131,11 @@
   RHS = EC->getSubExpr();
 if (!RHS)
   return nullptr;
-MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
-  return nullptr;
-return Field;
+if (MemberExpr *ME2 = dyn_cast(RHS)) {
+  if (ME2->getMemberDecl() == Field)
+return Field;
+}
+return nullptr;
   } else if (CXXMemberCallExpr *MCE = dyn_cast(S)) {
 CXXMethodDecl *MD = dyn_cast(MCE->getCalleeDecl());
 if (!(MD && isMemcpyEquivalentSpecialMember(MD)))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks, looks good.


https://reviews.llvm.org/D29208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[Warren Ristow via Phabricator via cfe-commits]

wristow marked 2 inline comments as done.
wristow added inline comments.



Comment at: lib/CodeGen/CGClass.cpp:1135
 MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
+if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field)
   return nullptr;

rjmccall wrote:
> wristow wrote:
> > rjmccall wrote:
> > > I would prefer:
> > > 
> > >   if (MemberExpr *ME2 = dyn_cast(RHS)) {
> > > if (ME2->getMemberDecl() == Field)
> > >   return Field;
> > >   }
> > >   return nullptr;
> > I see that change removes the `dyn_cast`.  Was that intended, or 
> > an oversight?
> > 
> > In terms of changing the code-structure, in code on it's own, I do like the 
> > approach you described.  But in this case, there is a sequence of `if 
> > () return nullptr; ... if (conditionN) return nullptr; return 
> > Field;`.  Then after the block containing that set of guarded `nullptr` 
> > returns with a final `return Field;`, there is a similar block.  And then 
> > there is a third block with a similar set.  So changing the structure in 
> > that way breaks that pattern.  With that in mind, do you still want that 
> > change done?
> The dyn_cast has no effect.  There is no situation in which the declarations 
> would compare equal without it where they would not with it, because Field is 
> already known to be a FieldDecl.
> 
> The structure of the existing code is unlikely to stay the same.  Actually, 
> that code is quite worrying — it's making a lot of assumptions about how Sema 
> synthesizes defaulted assignment operator bodies.  But I didn't want to ask 
> you to fix it when it's not the subject of your bug.
Got it.  Posted updated patch.


https://reviews.llvm.org/D29208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[Warren Ristow via Phabricator via cfe-commits]

wristow updated this revision to Diff 86767.
wristow added a comment.

Code restructured.


https://reviews.llvm.org/D29208

Files:
  lib/CodeGen/CGClass.cpp
  test/CodeGenCXX/atomic-dllexport.cpp


Index: test/CodeGenCXX/atomic-dllexport.cpp
===
--- test/CodeGenCXX/atomic-dllexport.cpp
+++ test/CodeGenCXX/atomic-dllexport.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++11 
-fms-extensions -O0 -o - %s | FileCheck --check-prefix=M32 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++11 
-fms-extensions -O0 -o - %s | FileCheck --check-prefix=M64 %s
+
+struct __declspec(dllexport) SomeStruct {
+  // Copy assignment operator should be produced, and exported:
+  // M32: define weak_odr dllexport x86_thiscallcc dereferenceable({{[0-9]+}}) 
%struct.SomeStruct* @"\01??4SomeStruct@@QAEAAU0@ABU0@@Z"
+  // M64: define weak_odr dllexportdereferenceable({{[0-9]+}}) 
%struct.SomeStruct* @"\01??4SomeStruct@@QEAAAEAU0@AEBU0@@Z"
+  _Atomic(int) mData;
+};
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -1131,10 +1131,11 @@
   RHS = EC->getSubExpr();
 if (!RHS)
   return nullptr;
-MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
-  return nullptr;
-return Field;
+if (MemberExpr *ME2 = dyn_cast(RHS)) {
+  if (ME2->getMemberDecl() == Field)
+return Field;
+}
+return nullptr;
   } else if (CXXMemberCallExpr *MCE = dyn_cast(S)) {
 CXXMethodDecl *MD = dyn_cast(MCE->getCalleeDecl());
 if (!(MD && isMemcpyEquivalentSpecialMember(MD)))


Index: test/CodeGenCXX/atomic-dllexport.cpp
===
--- test/CodeGenCXX/atomic-dllexport.cpp
+++ test/CodeGenCXX/atomic-dllexport.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++11 -fms-extensions -O0 -o - %s | FileCheck --check-prefix=M32 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++11 -fms-extensions -O0 -o - %s | FileCheck --check-prefix=M64 %s
+
+struct __declspec(dllexport) SomeStruct {
+  // Copy assignment operator should be produced, and exported:
+  // M32: define weak_odr dllexport x86_thiscallcc dereferenceable({{[0-9]+}}) %struct.SomeStruct* @"\01??4SomeStruct@@QAEAAU0@ABU0@@Z"
+  // M64: define weak_odr dllexportdereferenceable({{[0-9]+}}) %struct.SomeStruct* @"\01??4SomeStruct@@QEAAAEAU0@AEBU0@@Z"
+  _Atomic(int) mData;
+};
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -1131,10 +1131,11 @@
   RHS = EC->getSubExpr();
 if (!RHS)
   return nullptr;
-MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
-  return nullptr;
-return Field;
+if (MemberExpr *ME2 = dyn_cast(RHS)) {
+  if (ME2->getMemberDecl() == Field)
+return Field;
+}
+return nullptr;
   } else if (CXXMemberCallExpr *MCE = dyn_cast(S)) {
 CXXMethodDecl *MD = dyn_cast(MCE->getCalleeDecl());
 if (!(MD && isMemcpyEquivalentSpecialMember(MD)))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGClass.cpp:1135
 MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
+if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field)
   return nullptr;

wristow wrote:
> rjmccall wrote:
> > I would prefer:
> > 
> >   if (MemberExpr *ME2 = dyn_cast(RHS)) {
> > if (ME2->getMemberDecl() == Field)
> >   return Field;
> >   }
> >   return nullptr;
> I see that change removes the `dyn_cast`.  Was that intended, or 
> an oversight?
> 
> In terms of changing the code-structure, in code on it's own, I do like the 
> approach you described.  But in this case, there is a sequence of `if 
> () return nullptr; ... if (conditionN) return nullptr; return 
> Field;`.  Then after the block containing that set of guarded `nullptr` 
> returns with a final `return Field;`, there is a similar block.  And then 
> there is a third block with a similar set.  So changing the structure in that 
> way breaks that pattern.  With that in mind, do you still want that change 
> done?
The dyn_cast has no effect.  There is no situation in which the declarations 
would compare equal without it where they would not with it, because Field is 
already known to be a FieldDecl.

The structure of the existing code is unlikely to stay the same.  Actually, 
that code is quite worrying — it's making a lot of assumptions about how Sema 
synthesizes defaulted assignment operator bodies.  But I didn't want to ask you 
to fix it when it's not the subject of your bug.


https://reviews.llvm.org/D29208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[Warren Ristow via Phabricator via cfe-commits]

wristow added inline comments.



Comment at: lib/CodeGen/CGClass.cpp:1135
 MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
+if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field)
   return nullptr;

rjmccall wrote:
> I would prefer:
> 
>   if (MemberExpr *ME2 = dyn_cast(RHS)) {
> if (ME2->getMemberDecl() == Field)
>   return Field;
>   }
>   return nullptr;
I see that change removes the `dyn_cast`.  Was that intended, or an 
oversight?

In terms of changing the code-structure, in code on it's own, I do like the 
approach you described.  But in this case, there is a sequence of `if 
() return nullptr; ... if (conditionN) return nullptr; return 
Field;`.  Then after the block containing that set of guarded `nullptr` returns 
with a final `return Field;`, there is a similar block.  And then there is a 
third block with a similar set.  So changing the structure in that way breaks 
that pattern.  With that in mind, do you still want that change done?


https://reviews.llvm.org/D29208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added inline comments.
This revision now requires changes to proceed.



Comment at: lib/CodeGen/CGClass.cpp:1135
 MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
+if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field)
   return nullptr;

I would prefer:

  if (MemberExpr *ME2 = dyn_cast(RHS)) {
if (ME2->getMemberDecl() == Field)
  return Field;
  }
  return nullptr;


https://reviews.llvm.org/D29208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[Warren Ristow via Phabricator via cfe-commits]

wristow added a comment.

When a class that has been tagged as dllexport (for an MSVC target) contains an 
atomic data member via the C11 '_Atomic' approach, the front end crashes with a 
null pointer dereference.
This patch fixes it by guarding the null dereference with the approach used by 
similar code in the same method.


https://reviews.llvm.org/D29208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

[Warren Ristow via Phabricator via cfe-commits]

wristow created this revision.

Guard against a null pointer dereference that caused Clang to crash
when processing a class containing an _Atomic() data member,
and that is tagged with 'dllexport'.


https://reviews.llvm.org/D29208

Files:
  lib/CodeGen/CGClass.cpp
  test/CodeGenCXX/atomic-dllexport.cpp


Index: test/CodeGenCXX/atomic-dllexport.cpp
===
--- test/CodeGenCXX/atomic-dllexport.cpp
+++ test/CodeGenCXX/atomic-dllexport.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++11 
-fms-extensions -O0 -o - %s | FileCheck --check-prefix=M32 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++11 
-fms-extensions -O0 -o - %s | FileCheck --check-prefix=M64 %s
+
+struct __declspec(dllexport) SomeStruct {
+  // Copy assignment operator should be produced, and exported:
+  // M32: define weak_odr dllexport x86_thiscallcc dereferenceable({{[0-9]+}}) 
%struct.SomeStruct* @"\01??4SomeStruct@@QAEAAU0@ABU0@@Z"
+  // M64: define weak_odr dllexportdereferenceable({{[0-9]+}}) 
%struct.SomeStruct* @"\01??4SomeStruct@@QEAAAEAU0@AEBU0@@Z"
+  _Atomic(int) mData;
+};
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -1132,7 +1132,7 @@
 if (!RHS)
   return nullptr;
 MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
+if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field)
   return nullptr;
 return Field;
   } else if (CXXMemberCallExpr *MCE = dyn_cast(S)) {


Index: test/CodeGenCXX/atomic-dllexport.cpp
===
--- test/CodeGenCXX/atomic-dllexport.cpp
+++ test/CodeGenCXX/atomic-dllexport.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++11 -fms-extensions -O0 -o - %s | FileCheck --check-prefix=M32 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++11 -fms-extensions -O0 -o - %s | FileCheck --check-prefix=M64 %s
+
+struct __declspec(dllexport) SomeStruct {
+  // Copy assignment operator should be produced, and exported:
+  // M32: define weak_odr dllexport x86_thiscallcc dereferenceable({{[0-9]+}}) %struct.SomeStruct* @"\01??4SomeStruct@@QAEAAU0@ABU0@@Z"
+  // M64: define weak_odr dllexportdereferenceable({{[0-9]+}}) %struct.SomeStruct* @"\01??4SomeStruct@@QEAAAEAU0@AEBU0@@Z"
+  _Atomic(int) mData;
+};
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -1132,7 +1132,7 @@
 if (!RHS)
   return nullptr;
 MemberExpr *ME2 = dyn_cast(RHS);
-if (dyn_cast(ME2->getMemberDecl()) != Field)
+if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field)
   return nullptr;
 return Field;
   } else if (CXXMemberCallExpr *MCE = dyn_cast(S)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits