Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[arphaman]

Summary

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.

[rsmith]

lib/Sema/SemaOverload.cpp

11750 ↗

(On Diff #73343)

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.

[arphaman]

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?

[arphaman]

Ping

[arphaman]

Ping

[bruno]

LGTM!

lib/Sema/SemaExpr.cpp

11523 ↗

(On Diff #83822)

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

cfe/trunk/lib/Sema/SemaExpr.cpp

ExprResult Sema::BuildBinOp(Scope *S, SourceLocation OpLoc,
   // Handle pseudo-objects in the LHS.
   if (const BuiltinType *pty = LHSExpr->getType()->getAsPlaceholderType()) {
     // Assignments with a pseudo-object l-value need special analysis.
     if (pty->getKind() == BuiltinType::PseudoObject &&
         BinaryOperator::isAssignmentOp(Opc))
       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
       // that an overload set can be dependently-typed, but it never
       // instantiates to having an overloadable type.
       ExprResult resolvedRHS = CheckPlaceholderExpr(RHSExpr);
       if (resolvedRHS.isInvalid()) return ExprError();
       RHSExpr = resolvedRHS.get();
 
       if (RHSExpr->isTypeDependent() ||
           RHSExpr->getType()->isOverloadableType())
         return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
     }
         
     ExprResult LHS = CheckPlaceholderExpr(LHSExpr);
     if (LHS.isInvalid()) return ExprError();
     LHSExpr = LHS.get();
   }
 
   // Handle pseudo-objects in the RHS.
   if (const BuiltinType *pty = RHSExpr->getType()->getAsPlaceholderType()) {
     // 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);
 
     ExprResult resolvedRHS = CheckPlaceholderExpr(RHSExpr);
     if (!resolvedRHS.isUsable()) return ExprError();
     RHSExpr = resolvedRHS.get();
   }
 

cfe/trunk/test/Sema/PR28181.c

+// 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 '<overloaded function 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?}}