[clang] Do not crash on use of a variadic overloaded operator

[Fznamznon]

Summary

Just exit instead.
Fixes https://github.com/llvm/llvm-project/issues/42535

[shafik]

clang/lib/Sema/SemaOverload.cpp

14006

It feels a bit weird that we are succeeding in finding the best viable function and what we find is invalid.

[Fznamznon]

clang/lib/Sema/SemaOverload.cpp

14006

Yes, but it seems to be done this way in other places as well:
https://github.com/llvm/llvm-project/blob/0478ef2d366c6f88678e37d54190dcdaa0ec69da/clang/lib/Sema/SemaOverload.cpp#L15145 .

[shafik]

clang/lib/Sema/SemaOverload.cpp

14006

It looks like we have a couple of test that generate the cannot be variadic diagnostic for overloads e.g. clang/test/SemaCXX/overloaded-operator-decl.cpp it might be worth looking into why they don't crash and this case does.

[shafik]

clang/lib/Sema/SemaOverload.cpp

14006

I see that but that diagnostic looks like it is generated by the call to CheckMemberOperatorAccess(...) which is not really the same situation we have here.

[Fznamznon]

clang/lib/Sema/SemaOverload.cpp

14006

We crash when ask functiondecl its second parameter and it doesn't have it when the call expression is formed.
The cases from tests do not crash either because the operators are not used or there is at least two parameters defined in declaration of operator.

[Fznamznon]

clang/lib/Sema/SemaOverload.cpp

14006

I see that but that diagnostic looks like it is generated by the call to CheckMemberOperatorAccess(...) which is not really the same situation we have here.

Is it? For me it seems there is a similar exit inside of BuildCallToObjectOfClassType function whose description says:

/// BuildCallToObjectOfClassType - Build a call to an object of class
/// type (C++ [over.call.object]), which can end up invoking an
/// overloaded function call operator (@c operator()) or performing a
/// user-defined conversion on the object argument.

CheckMemberOperatorAccess is just a call above.

[aaron.ballman]

clang/lib/Sema/SemaOverload.cpp

14006

It feels a bit weird that we are succeeding in finding the best viable function and what we find is invalid.

I don't think it's all that weird. Consider:

void overloaded(int);
void overloaded(float) noexcept(error());

int main() {
  overloaded(1.0f);
}

the best viable function is definitely the one taking a float, but the declaration itself is still invalid.

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

I think it might make sense to extend the test coverage for the other operators you can overload, just to demonstrate we diagnose them all consistently. WDYT?

[shafik]

clang/lib/Sema/SemaOverload.cpp

14006

It feels a bit weird that we are succeeding in finding the best viable function and what we find is invalid.

I don't think it's all that weird.

I think the fact that we have cases for which this diagnostic fires but does not crash deserved some digging into to see why those cases avoid this situation. If it ends up they are doing something similar then we can feel more confident that this is the right approach.

[aaron.ballman]

clang/lib/Sema/SemaOverload.cpp

14006

I thought we already knew what was going on there?

The cases from tests do not crash either because the operators are not used or there is at least two parameters defined in declaration of operator.

[Fznamznon]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

Okay, while trying to add more test cases I discovered that following

class E {};
bool operator<(const E& lhs, ...);
auto operator<=>(const E& lhs, ...);

void d() {
  E() < E();
}

crashes even with the patch since there is code searching for best overload candidate that doesn't consider possibility for them making variadic.
The code around overloading is actually pretty inconsistent, somewhere invalid candidates are considered, and somewhere not, so I spent some time not knowing what to do.
I'm now inclined that we just shouldn't consider invalid candidates like @shafik
suggests. WDYY?

[aaron.ballman]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

Overload resolution doesn't need to produce a candidate that's viable to call; C++ lets you resolve to the "best viable function" only to say "and that one wasn't good enough either." e.g., http://eel.is/c++draft/over#match.general-3

I've not yet spotted anything in http://eel.is/c++draft/over that says invalid declarations should/should not be added to the initial candidate set. I *think* the intention is that if name lookup can find the name, it goes into the candidate set. Then that set is processed to remove functions that are not viable (http://eel.is/c++draft/over.match.viable). Then we find the best viable function from that set.

I think we should be keeping the function in the candidate set so long as it matches the rules in http://eel.is/c++draft/over.match.viable even if the function is otherwise not viable. Otherwise, correcting an unrelated issue might change overload resolution to find a completely different function. e.g., in my example above, we'd select void overloaded(int); as the best viable function, but when the user corrects the float function, we'd change to call that instead. I think it's easier to understand what's going on when picking the float overload to begin with and saying "but we can't call that because it's busted".

CC @cor3ntin @hubert.reinterpretcast @rsmith for some extra opinions, as I'm not certain if I'm interpreting the standard correctly or not.

[cor3ntin]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

I'm not sure how much the standardese matter here as the declaration of the operators are ill-formed anyway.
But from a QOI perspective, i think keeping in the set everything the users might reasonably expect to be considered makes sense to me, as it *should* lead to better diagnostics

[aaron.ballman]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

Thanks for the extra perspective! Yeah, I think that's what I've been convincing myself of. Effectively:

int overloaded(int);
float overloaded(float) noexcept(error()); // error: invalid declaration

int main() {
  (void)overloaded(1.0f); // #1
}

We're always going to get the invalid declaration diagnostic. The question is whether we want a diagnostic at #1 that says "can't call this overload" or whether we want #1 to have no diagnostic (because it picked the int overload). From a purely diagnostic perspective, I think I can see arguments either way. But if we change it slightly:

template <typename Ty>
constexpr int func() { return 0; }

template <>
constexpr int func<int>() { return 1; }

template <>
constexpr int func<float>() { return 2; }

int overloaded(int);
float overloaded(float) noexcept(error()); // error: invalid declaration

static_assert(func(overloaded(1.0f)) == 2); // #1

we still get the invalid declaration diagnostic, but now we get a failing static assertion because we picked the int overload rather than the float overload. I think this is clearly worse behavior than if we issued an additional diagnostic at #1 saying that the overload we picked (float) was invalid.

https://godbolt.org/z/34aGMY43n

WDYT?

[cor3ntin]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

Agreed. If i write an overload i presumably want the compiler to consider it, even if I botched it. And apparently, the issue is not that we don't do that but rather than we don't always do that. We should strive to be consistent.

It's not critical because even in the static_assert case, the only thing a user can do is fix the declaration, at which point they would get the ambiguity diag or the correct best match correctly picked. But it's nicer to have more diags, when we can

[Fznamznon]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

I see, thank you both for the extensive explanation.

[Fznamznon]

Rebase, add more test cases, fix assertion on variadic functions inside of
haveSameParameterTypes

[Fznamznon]

clang/test/SemaCXX/overloaded-operator-decl.cpp

65

Ok, added a bit more test cases.

[aaron.ballman]

LGTM!

[Fznamznon]

Hmm, clang-format reports failure, but applying git-clang-format to the commit doesn't give me anything. It seems the whole SemaOverload.cpp file has broken formatting, maybe that is the reason for the failure.

clang/docs/ReleaseNotes.rst

- Clang constexpr evaluator now prints template arguments when displaying
   template-specialization function calls.
 
 Bug Fixes in This Version
 -------------------------
 - Fixed an issue where a class template specialization whose declaration is
   instantiated in one module and whose definition is instantiated in another
   module may end up with members associated with the wrong declaration of the
   class, which can result in miscompiles in some cases.
+- Fix crash on use of a variadic overloaded operator.
+  (`#42535 <https://github.com/llvm/llvm-project/issues/42535>_`)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Sema/SemaOverload.cpp

getImplicitObjectParamType(ASTContext &Context, const FunctionDecl *F) {
 
   QualType T = M->getThisObjectType();
   if (M->getRefQualifier() == RQ_RValue)
     return Context.getRValueReferenceType(T);
   return Context.getLValueReferenceType(T);
 }
 
 static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
-                                   const FunctionDecl *F2, unsigned NumParams) {
+                                   const FunctionDecl *F2) {
   if (declaresSameEntity(F1, F2))
     return true;
 
   auto NextParam = [&](const FunctionDecl *F, unsigned &I, bool First) {
     if (First) {
       if (std::optional<QualType> T = getImplicitObjectParamType(Context, F))
         return *T;
     }
     assert(I < F->getNumParams());
     return F->getParamDecl(I++)->getType();
   };
 
+  unsigned F1NumParams = F1->getNumParams() + isa<CXXMethodDecl>(F1);
+  unsigned F2NumParams = F2->getNumParams() + isa<CXXMethodDecl>(F2);
+
+  if (F1NumParams != F2NumParams)
+    return false;
+
   unsigned I1 = 0, I2 = 0;
-  for (unsigned I = 0; I != NumParams; ++I) {
+  for (unsigned I = 0; I != F1NumParams; ++I) {
     QualType T1 = NextParam(F1, I1, I == 0);
     QualType T2 = NextParam(F2, I2, I == 0);
     assert(!T1.isNull() && !T2.isNull() && "Unexpected null param types");
     if (!Context.hasSameUnqualifiedType(T1, T2))
       return false;
   }
   return true;
 }
for (unsigned ArgIdx = StartArg; ArgIdx < NumArgs; ++ArgIdx) {
     case ImplicitConversionSequence::Better:
       // Cand1 has a better conversion sequence.
       HasBetterConversion = true;
       break;
 
     case ImplicitConversionSequence::Worse:
       if (Cand1.Function && Cand2.Function &&
           Cand1.isReversed() != Cand2.isReversed() &&
-          haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function,
-                                 NumArgs)) {
+          haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function)) {
         // Work around large-scale breakage caused by considering reversed
         // forms of operator== in C++20:
         //
         // When comparing a function against a reversed function with the same
         // parameter types, if we have a better conversion for one argument and
         // a worse conversion for the other, the implicit conversion sequences
         // are treated as being equally good.
         //
case OR_Success: {
       // We found a built-in operator or an overloaded operator.
       FunctionDecl *FnDecl = Best->Function;
 
       bool IsReversed = Best->isReversed();
       if (IsReversed)
         std::swap(Args[0], Args[1]);
 
       if (FnDecl) {
+
+        if (FnDecl->isInvalidDecl())
+          return ExprError();
+
         Expr *Base = nullptr;
         // We matched an overloaded operator. Build a call to that
         // operator.
 
         OverloadedOperatorKind ChosenOp =
             FnDecl->getDeclName().getCXXOverloadedOperator();
 
         // C++2a [over.match.oper]p9:
case OR_Success: {
             CandidateSet.getRewriteInfo().isReversible()) {
           // We could have reversed this operator, but didn't. Check if some
           // reversed form was a viable candidate, and if so, if it had a
           // better conversion for either parameter. If so, this call is
           // formally ambiguous, and allowing it is an extension.
           llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
           for (OverloadCandidate &Cand : CandidateSet) {
             if (Cand.Viable && Cand.Function && Cand.isReversed() &&
-                haveSameParameterTypes(Context, Cand.Function, FnDecl, 2)) {
+                haveSameParameterTypes(Context, Cand.Function, FnDecl)) {
               for (unsigned ArgIdx = 0; ArgIdx < 2; ++ArgIdx) {
                 if (CompareImplicitConversionSequences(
                         *this, OpLoc, Cand.Conversions[ArgIdx],
                         Best->Conversions[ArgIdx]) ==
                     ImplicitConversionSequence::Better) {
                   AmbiguousWith.push_back(Cand.Function);
                   break;
                 }

clang/test/SemaCXX/overloaded-operator-decl.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s 
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
 struct X { 
   X();
   X(int); 
 };
 
 X operator+(X, X);
 X operator-(X, X) { X x; return x; }
 
namespace PR14120 {
   struct A {
     static void operator()(int& i) { ++i; } // expected-warning{{is a C++23 extension}}
   };
   void f() {
     int i = 0;
     A()(i);
   }
 }
+
+namespace GH42535 {
+class E {
+  E& operator=(const E& rhs, ...); // expected-error{{overloaded 'operator=' cannot be variadic}}
+  E& operator+=(const E& rhs, ...); // expected-error{{overloaded 'operator+=' cannot be variadic}}
+
+};
+void operator+(E, ...) {} // expected-error{{overloaded 'operator+' cannot be variadic}}
+void operator-(E, ...) {} // expected-error{{overloaded 'operator-' cannot be variadic}}
+void operator*(E, ...) {} // expected-error{{overloaded 'operator*' cannot be variadic}}
+void operator/(E, ...) {} // expected-error{{overloaded 'operator/' must be a binary operator}}
+void operator/(E, E, ...) {} // expected-error{{overloaded 'operator/' cannot be variadic}}
+void operator%(E, ...) {} // expected-error{{overloaded 'operator%' must be a binary operator}}
+void operator%(E, E, ...) {} // expected-error{{overloaded 'operator%' cannot be variadic}}
+E& operator++(E&, ...); // expected-error{{overloaded 'operator++' cannot be variadic}}
+E& operator--(E&, ...); // expected-error{{overloaded 'operator--' cannot be variadic}}
+bool operator<(const E& lhs, ...); // expected-error{{overloaded 'operator<' must be a binary operator}}
+bool operator<(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator>(const E& lhs, ...); // expected-error{{overloaded 'operator>' must be a binary operator}}
+bool operator>(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator>=(const E& lhs, ...); // expected-error{{overloaded 'operator>=' must be a binary operator}}
+bool operator>=(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator<=(const E& lhs, ...); // expected-error{{overloaded 'operator<=' must be a binary operator}}
+bool operator<=(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator==(const E& lhs, ...); // expected-error{{overloaded 'operator==' must be a binary operator}}
+bool operator==(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator!=(const E& lhs, ...); // expected-error{{overloaded 'operator!=' must be a binary operator}}
+bool operator!=(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator&&(const E& lhs, ...); // expected-error{{overloaded 'operator&&' must be a binary operator}}
+bool operator&&(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator||(const E& lhs, ...); // expected-error{{overloaded 'operator||' must be a binary operator}}
+bool operator||(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator>>(const E& lhs, ...); // expected-error{{overloaded 'operator>>' must be a binary operator}}
+bool operator>>(const E& lhs, const E& rhs, ...); // expected-error{{cannot be variadic}}
+bool operator&(const E& lhs, ...); // expected-error{{cannot be variadic}}
+#if __cplusplus >= 202002L
+auto operator<=>(const E& lhs, ...);  // expected-error{{overloaded 'operator<=>' must be a binary operator}}
+#endif
+void d() {
+  E() + E();
+  E() - E();
+  E() * E();
+  E() / E();
+  E() % E();
+  ++E(); // expected-error{{cannot increment value of type 'E'}}
+  --E(); // expected-error{{cannot decrement value of type 'E'}}
+  E() < E();
+  E() > E();
+  E() <= E();
+  E() >= E();
+  E() == E();
+  E() != E();
+#if __cplusplus >= 202002L
+  E() <=> E();
+#endif
+  E e;
+  E e1 = e;
+  e += e1;
+  E() && E();
+  E() || E();
+  E() & E();
+  E() >> E();
+}
+}