[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Alex Lorenz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.
Closed by commit rL292497: [Sema] Fix PR28181 by avoiding calling 
BuildOverloadedBinOp in C mode (authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D25213?vs=83822=84983#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25213

Files:
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Sema/PR28181.c


Index: cfe/trunk/test/Sema/PR28181.c
===
--- cfe/trunk/test/Sema/PR28181.c
+++ cfe/trunk/test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct 
spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded 
function could not be resolved; did you mean to call it?}}
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -11505,7 +11505,7 @@
   return checkPseudoObjectAssignment(S, OpLoc, Opc, LHSExpr, RHSExpr);
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload) {
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload) {
   // We can't actually test that if we still have a placeholder,
   // though.  Fortunately, none of the exceptions we see in that
   // code below are valid when the LHS is an overload set.  Note
@@ -11530,17 +11530,16 @@
 // An overload in the RHS can potentially be resolved by the type
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
-  if (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())
-return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
-
-  if (LHSExpr->getType()->isOverloadableType())
+  if (getLangOpts().CPlusPlus &&
+  (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent() ||
+   LHSExpr->getType()->isOverloadableType()))
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
   return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
 }
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload &&
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload &&
 LHSExpr->getType()->isOverloadableType())
   return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 


Index: cfe/trunk/test/Sema/PR28181.c
===
--- cfe/trunk/test/Sema/PR28181.c
+++ cfe/trunk/test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded function could not be resolved; did you mean to call it?}}
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -11505,7 +11505,7 @@
   return checkPseudoObjectAssignment(S, OpLoc, Opc, LHSExpr, RHSExpr);
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload) {
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload) {
   // We can't actually test that if we still have a placeholder,
   // though.  Fortunately, none of the exceptions we see in that
   // code below are valid when the LHS is an overload set.  Note
@@ -11530,17 +11530,16 @@
 // An overload in the RHS can potentially be resolved by the type
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
-  if (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())
-return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
-
-  if (LHSExpr->getType()->isOverloadableType())
+  if (getLangOpts().CPlusPlus &&
+  (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent() ||
+   

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM!




Comment at: lib/Sema/SemaExpr.cpp:11523
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
+  if (getLangOpts().CPlusPlus &&

Looks like you can fold both conditions below into one and check 
`getLangOpts().CPlusPlus` only once


Repository:
  rL LLVM

https://reviews.llvm.org/D25213



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

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Ping


Repository:
  rL LLVM

https://reviews.llvm.org/D25213



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

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 83822.
arphaman added a comment.

Ping


Repository:
  rL LLVM

https://reviews.llvm.org/D25213

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/PR28181.c


Index: test/Sema/PR28181.c
===
--- /dev/null
+++ test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct 
spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded 
function could not be resolved; did you mean to call it?}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11496,7 +11496,7 @@
   return checkPseudoObjectAssignment(S, OpLoc, Opc, LHSExpr, RHSExpr);
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload) {
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload) {
   // We can't actually test that if we still have a placeholder,
   // though.  Fortunately, none of the exceptions we see in that
   // code below are valid when the LHS is an overload set.  Note
@@ -11521,17 +11521,18 @@
 // An overload in the RHS can potentially be resolved by the type
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
-  if (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())
+  if (getLangOpts().CPlusPlus &&
+  (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent()))
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
-  if (LHSExpr->getType()->isOverloadableType())
+  if (getLangOpts().CPlusPlus && LHSExpr->getType()->isOverloadableType())
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
   return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
 }
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload &&
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload &&
 LHSExpr->getType()->isOverloadableType())
   return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 


Index: test/Sema/PR28181.c
===
--- /dev/null
+++ test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded function could not be resolved; did you mean to call it?}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11496,7 +11496,7 @@
   return checkPseudoObjectAssignment(S, OpLoc, Opc, LHSExpr, RHSExpr);
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload) {
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload) {
   // We can't actually test that if we still have a placeholder,
   // though.  Fortunately, none of the exceptions we see in that
   // code below are valid when the LHS is an overload set.  Note
@@ -11521,17 +11521,18 @@
 // An overload in the RHS can potentially be resolved by the type
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
-  if (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())
+  if (getLangOpts().CPlusPlus &&
+  (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent()))
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
-  if (LHSExpr->getType()->isOverloadableType())
+  if (getLangOpts().CPlusPlus && LHSExpr->getType()->isOverloadableType())
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
   return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
 }
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload &&
+if (getLangOpts().CPlusPlus && pty->getKind() == 

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Alex Lorenz via cfe-commits]

arphaman updated this revision to Diff 73460.
arphaman marked an inline comment as done.
arphaman added a comment.

Thanks for the response,

I updated the patch with a approach that you suggested - now 
`Sema::CreateOverloadedBinOp` isn't called in C mode (I have an assertion for 
this in `Sema::CreateOverloadedBinOp`, but I'll commit it separately after).

I also tried moving the CorrectDelayedTyposInExpr code from 
`Sema::CreateBuiltinBinOp` to `Sema::BuildBinOp` as you suggested, but it 
didn't seem to have any effect whatsoever - it didn't seem to change the 
behavior of anything with respect to this bug or llvm's test suite. Therefore, 
I decided to leave it in its original location in this diff. Perhaps I've 
misunderstood your suggestion?


Repository:
  rL LLVM

https://reviews.llvm.org/D25213

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/PR28181.c


Index: test/Sema/PR28181.c
===
--- /dev/null
+++ test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct 
spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded 
function could not be resolved; did you mean to call it?}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11391,7 +11391,7 @@
   return checkPseudoObjectAssignment(S, OpLoc, Opc, LHSExpr, RHSExpr);
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload) {
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload) {
   // We can't actually test that if we still have a placeholder,
   // though.  Fortunately, none of the exceptions we see in that
   // code below are valid when the LHS is an overload set.  Note
@@ -11416,17 +11416,18 @@
 // An overload in the RHS can potentially be resolved by the type
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
-  if (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())
+  if (getLangOpts().CPlusPlus &&
+  (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent()))
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
-  if (LHSExpr->getType()->isOverloadableType())
+  if (getLangOpts().CPlusPlus && LHSExpr->getType()->isOverloadableType())
 return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 
   return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
 }
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload &&
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload &&
 LHSExpr->getType()->isOverloadableType())
   return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
 


Index: test/Sema/PR28181.c
===
--- /dev/null
+++ test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded function could not be resolved; did you mean to call it?}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11391,7 +11391,7 @@
   return checkPseudoObjectAssignment(S, OpLoc, Opc, LHSExpr, RHSExpr);
 
 // Don't resolve overloads if the other type is overloadable.
-if (pty->getKind() == BuiltinType::Overload) {
+if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload) {
   // We can't actually test that if we still have a placeholder,
   // though.  Fortunately, none of the exceptions we see in that
   // code below are valid when the LHS is an overload set.  Note
@@ -11416,17 +11416,18 @@
 // An overload in the RHS can potentially be resolved by the type
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
-  if (LHSExpr->isTypeDependent() || 

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Richard Smith via cfe-commits]

rsmith added inline comments.


> SemaOverload.cpp:11750
>  ExprResult
>  Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
>  BinaryOperatorKind Opc,

We should never be calling this function outside of C++ mode.

It looks like the bug is in `Sema::BuildBinOp` -- in C, it should call 
`CorrectDelayedTyposInExpr` before doing almost anything else. The relevant 
code is currently in `CreateBuiltinBinOp`, which is too late. Moving that code 
to the start of `BuildBinOp` should solve the problem.

Repository:
  rL LLVM

https://reviews.llvm.org/D25213



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

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Alex Lorenz via cfe-commits]

arphaman created this revision.
arphaman added reviewers: rsmith, rjmccall.
arphaman added a subscriber: cfe-commits.
arphaman set the repository for this revision to rL LLVM.

This patch fixes a crash that happens because of an assertion failure in C mode 
only. The assertion failure happens because of a cast to a C++ record decl in 
code that assumes (correctly, AFAIK) that overloaded operator handling is used 
in C++ mode only.

This patch approaches this problem in the following manner: when clang isn't in 
C++ mode, it exits early from the method 'Sema::CreateOverloadedBinOp'. The 
operator handling is performed using the 'Sema::CreateBuiltinBinOp' method when 
exiting the previously mentioned method. I think that this approach works 
because, AFAIK, clang doesn't support operator overloading without 
LangOpts.CPlusPlus, so it doesn't need to handle the overloading in the same 
way, and redirecting to builtin operator handling deals with the C operators 
correctly. I also tried other approaches and strategies but they didn't seem to 
work as well as this one.

I'm not familiar with all the intricacies of Sema, and I realize that this 
might be the wrong approach for this problem. Please let me know if this a 
sound strategy or not.

This bug is a regression since r236337.


Repository:
  rL LLVM

https://reviews.llvm.org/D25213

Files:
  lib/Sema/SemaOverload.cpp
  test/Sema/PR28181.c


Index: test/Sema/PR28181.c
===
--- /dev/null
+++ test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct 
spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 
'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded 
function could not be resolved; did you mean to call it?}}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -11804,7 +11804,8 @@
   // various built-in candidates, but as DR507 points out, this can lead to
   // problems. So we do it this way, which pretty much follows what GCC does.
   // Note that we go the traditional code path for compound assignment forms.
-  if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
+  if (!LangOpts.CPlusPlus ||
+  (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType()))
 return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
 
   // If this is the .* operator, which is not overloadable, just


Index: test/Sema/PR28181.c
===
--- /dev/null
+++ test/Sema/PR28181.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct spinlock_t {
+  int lock;
+} audit_skb_queue;
+
+void fn1() {
+  audit_skb_queue = (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{assigning to 'struct spinlock_t' from incompatible type ''}}
+
+void fn2() {
+  audit_skb_queue + (lock); // expected-error {{use of undeclared identifier 'lock'; did you mean 'long'?}}
+}   // expected-error@-1 {{reference to overloaded function could not be resolved; did you mean to call it?}}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -11804,7 +11804,8 @@
   // various built-in candidates, but as DR507 points out, this can lead to
   // problems. So we do it this way, which pretty much follows what GCC does.
   // Note that we go the traditional code path for compound assignment forms.
-  if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
+  if (!LangOpts.CPlusPlus ||
+  (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType()))
 return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
 
   // If this is the .* operator, which is not overloadable, just
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits