[clang] Add missing diagnostics for invalid overloads of multiversion functions in C.

[tahonermann]

Summary

Previously, an attempt to declare an overload of a multiversion function
in C was not properly diagnosed. In some cases, diagnostics were simply
missing. In other cases the following assertion failure occured...

Assertion `(Previous.empty() || llvm::any_of(Previous, [](const NamedDecl *ND) { return ND->hasAttr(); })) && "Non-redecls shouldn't happen without overloadable present"' failed.

... or the following diagnostic was spuriously issued.

error: at most one overload for a given name may lack the 'overloadable' attribute

The diagnostics issued in some cases could be improved. When the function
type of a redeclaration does not match the prior declaration, it would be
preferable to diagnose the type mismatch before diagnosing mismatched
attributes. Diagnostics are also missing for some cases.

[tahonermann]

This change resolves both LLVM issue https://github.com/llvm/llvm-project/issues/50192 and Intel issue CMPLRLLVM-28177.

[erichkeane]

In D121959#3390658, @tahonermann wrote:

Intel issue CMPLRLLVM-28177.

These words have no power here.

[erichkeane]

Generally looks fine to me. Just 1 test that looks suspicious.

clang/test/Sema/attr-cpuspecific.c

133

In what way is this test different than bad_overload1?

[tahonermann]

These words have no power here.

No power, but not uninformative to some. I promise to keep such references out of git's permanent record :)

clang/test/Sema/attr-cpuspecific.c

133

bad_overload1 has cpu_specific in both declarations while the declarations for bad_overload3 exercise both cpu_dispatch and cpu_specific.

[erichkeane]

LGTM, thanks for the clarification!

clang/test/Sema/attr-cpuspecific.c

133

Ah!

[tahonermann]

Rebase.

[tahonermann]

Corrected a clang-format issue.

[dblaikie]

This might've caused a regression? (https://github.com/llvm/llvm-project/issues/54892)

[tahonermann]

This might've caused a regression? (https://github.com/llvm/llvm-project/issues/54892)

@dblaikie, it most definitely did. Note that I'm the author of the patch that introduced the regression, the reporter of the regression, and the assignee for fixing the regression ;)

[dblaikie]

In D121959#3454606, @tahonermann wrote:

This might've caused a regression? (https://github.com/llvm/llvm-project/issues/54892)

@dblaikie, it most definitely did. Note that I'm the author of the patch that introduced the regression, the reporter of the regression, and the assignee for fixing the regression ;)

Ah, sorry, didn't check the author/folks involved. It's not uncommon for non-contributors to file bugs for new breakages and for contributors not to be monitoring the bugs that closely - so usually a conversation on the commit/review is a better place for visibility - so was passing it along to make sure it didn't get lost :)

clang/lib/Sema/SemaDecl.cpp

 void Sema::ActOnExitFunctionContext() {
   // Same implementation as PopDeclContext, but returns to the lexical parent,
   // rather than the top-level class.
   assert(CurContext && "DeclContext imbalance!");
   CurContext = CurContext->getLexicalParent();
   assert(CurContext && "Popped translation unit!");
 }
 
-/// Determine whether we allow overloading of the function
-/// PrevDecl with another declaration.
+/// Determine whether overloading is allowed for a new function
+/// declaration considering prior declarations of the same name.
 ///
 /// This routine determines whether overloading is possible, not
-/// whether some new function is actually an overload. It will return
-/// true in C++ (where we can always provide overloads) or, as an
-/// extension, in C when the previous function is already an
-/// overloaded function declaration or has the "overloadable"
-/// attribute.
-static bool AllowOverloadingOfFunction(LookupResult &Previous,
+/// whether a new declaration actually overloads a previous one.
+/// It will return true in C++ (where overloads are alway permitted)
+/// or, as a C extension, when either the new declaration or a
+/// previous one is declared with the 'overloadable' attribute.
+static bool AllowOverloadingOfFunction(const LookupResult &Previous,
                                        ASTContext &Context,
                                        const FunctionDecl *New) {
-  if (Context.getLangOpts().CPlusPlus)
+  if (Context.getLangOpts().CPlusPlus || New->hasAttr<OverloadableAttr>())
     return true;
 
-  if (Previous.getResultKind() == LookupResult::FoundOverloaded)
-    return true;
+  // Multiversion function declarations are not overloads in the
+  // usual sense of that term, but lookup will report that an
+  // overload set was found if more than one multiversion function
+  // declaration is present for the same name. It is therefore
+  // inadequate to assume that some prior declaration(s) had
+  // the overloadable attribute; checking is required. Since one
+  // declaration is permitted to omit the attribute, it is necessary
+  // to check at least two; hence the 'any_of' check below. Note that
+  // the overloadable attribute is implicitly added to declarations
+  // that were required to have it but did not.
+  if (Previous.getResultKind() == LookupResult::FoundOverloaded) {
+    return llvm::any_of(Previous, [](const NamedDecl *ND) {
+      return ND->hasAttr<OverloadableAttr>();
+    });
+  } else if (Previous.getResultKind() == LookupResult::Found)
+    return Previous.getFoundDecl()->hasAttr<OverloadableAttr>();
 
-  return Previous.getResultKind() == LookupResult::Found &&
-         (Previous.getFoundDecl()->hasAttr<OverloadableAttr>() ||
-          New->hasAttr<OverloadableAttr>());
+  return false;
 }
 
 /// Add this decl to the scope shadowed decl chains.
 void Sema::PushOnScopeChains(NamedDecl *D, Scope *S, bool AddToContext) {
   // Move up the scope chain until we find the nearest enclosing
   // non-transparent context. The declaration will be introduced into this
   // scope.
   while (S->getEntity() && S->getEntity()->isTransparentContext())
static bool CheckMultiVersionAdditionalDecl(
   if (NewTA) {
     NewParsed = NewTA->parse();
     llvm::sort(NewParsed.Features);
   }
 
   bool UseMemberUsingDeclRules =
       S.CurContext->isRecord() && !NewFD->getFriendObjectKind();
 
+  bool MayNeedOverloadableChecks =
+      AllowOverloadingOfFunction(Previous, S.Context, NewFD);
+
   // Next, check ALL non-overloads to see if this is a redeclaration of a
   // previous member of the MultiVersion set.
   for (NamedDecl *ND : Previous) {
     FunctionDecl *CurFD = ND->getAsFunction();
     if (!CurFD)
       continue;
-    if (S.IsOverload(NewFD, CurFD, UseMemberUsingDeclRules))
+    if (MayNeedOverloadableChecks &&
+        S.IsOverload(NewFD, CurFD, UseMemberUsingDeclRules))
       continue;
 
     switch (NewMVType) {
     case MultiVersionKind::None:
       assert(OldMVType == MultiVersionKind::TargetClones &&
              "Only target_clones can be omitted in subsequent declarations");
       break;
     case MultiVersionKind::Target: {

clang/test/Sema/attr-cpuspecific.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -Wno-strict-prototypes -fsyntax-only -verify %s -Wnonnull
-// XFAIL: asserts
 
 void __attribute__((cpu_specific(ivybridge))) no_default(void);
 void __attribute__((cpu_specific(sandybridge)))  no_default(void);
 
 void use1(void){
   // Should be OK, default not a problem.
   no_default();
 }
int use3(void) {
   return called_invalid_value();
 }
 
 // expected-warning@+1 {{CPU list contains duplicate entries; attribute ignored}}
 int __attribute__((cpu_dispatch(pentium_iii, pentium_iii_no_xmm_regs))) dupe_p3(void);
 
 void __attribute__((cpu_specific(atom), nothrow, nonnull(1))) addtl_attrs(int*);
 
-// FIXME: Declaration of a non-overloadable function when more than one
-// FIXME: multiversion function declarations are present results in an
-// FIXME: assertion failure.
 int __attribute__((cpu_specific(atom))) bad_overload1(void);
 int __attribute__((cpu_specific(ivybridge))) bad_overload1(void);
-// expected-error@+2 {{at most one overload for a given name may lack the 'overloadable' attribute}}
-// expected-note@-2 {{previous unmarked overload of function is here}}
+// expected-error@+1 {{function declaration is missing 'cpu_specific' or 'cpu_dispatch' attribute in a multiversioned function}}
 int bad_overload1(int);
 
 int bad_overload2(int);
 // expected-error@+2 {{conflicting types for 'bad_overload2'}}
 // expected-note@-2 {{previous declaration is here}}
 int __attribute__((cpu_specific(atom))) bad_overload2(void);
 int __attribute__((cpu_specific(ivybridge))) bad_overload2(void);
 
-// FIXME: Declaration of a non-overloadable function when more than one
-// FIXME: multiversion function declarations are present results in an
-// FIXME: assertion failure.
 int __attribute__((cpu_dispatch(generic))) bad_overload3(void);
 int __attribute__((cpu_specific(ivybridge))) bad_overload3(void);
-// expected-error@+2 {{at most one overload for a given name may lack the 'overloadable' attribute}}
-// expected-note@-2 {{previous unmarked overload of function is here}}
+// expected-error@+1 {{function declaration is missing 'cpu_specific' or 'cpu_dispatch' attribute in a multiversioned function}}
 int bad_overload3(int);
 
 int bad_overload4(int);
 // expected-error@+2 {{conflicting types for 'bad_overload4'}}
 // expected-note@-2 {{previous declaration is here}}
 int __attribute__((cpu_dispatch(generic))) bad_overload4(void);
 int __attribute__((cpu_specific(ivybridge))) bad_overload4(void);
 

clang/test/Sema/attr-target-clones.c

 }
 // expected-error@+1 {{function declaration cannot become a multiversioned function after first usage}}
 int __attribute__((target_clones("sse4.2", "default"))) mv_after_use(void) { return 1; }
 
 void bad_overload1(void) __attribute__((target_clones("mmx", "sse4.2", "default")));
 void bad_overload1(int p) {}
 
 void bad_overload2(int p) {}
+// expected-error@+2 {{conflicting types for 'bad_overload2'}}
+// expected-note@-2 {{previous definition is here}}
 void bad_overload2(void) __attribute__((target_clones("mmx", "sse4.2", "default")));
 
 void bad_overload3(void) __attribute__((target_clones("mmx", "sse4.2", "default")));
+// expected-error@+2 {{conflicting types for 'bad_overload3'}}
+// expected-note@-2 {{previous declaration is here}}
 void bad_overload3(int) __attribute__((target_clones("mmx", "sse4.2", "default")));
 
 void good_overload1(void) __attribute__((target_clones("mmx", "sse4.2", "default")));
 void __attribute__((__overloadable__)) good_overload1(int p) {}
 
 // expected-error@+1 {{attribute 'target_clones' multiversioning cannot be combined with attribute 'overloadable'}}
 void __attribute__((__overloadable__)) good_overload2(void) __attribute__((target_clones("mmx", "sse4.2", "default")));
 void good_overload2(int p) {}

clang/test/Sema/attr-target-mv.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -Wno-strict-prototypes -fsyntax-only -verify %s
-// XFAIL: asserts
 
 void __attribute__((target("sse4.2"))) no_default(void);
 void __attribute__((target("arch=sandybridge")))  no_default(void);
 
 void use1(void){
   // expected-error@+1 {{no matching function for call to 'no_default'}}
   no_default();
 }

 short __attribute__((target("arch=sandybridge")))  diff_ret(void);
 
 void __attribute__((target("sse4.2"), nothrow, used, nonnull(1))) addtl_attrs5(int*);
 void __attribute__((target("arch=sandybridge"))) addtl_attrs5(int*);
 
 void __attribute__((target("sse4.2"))) addtl_attrs6(int*);
 void __attribute__((target("arch=sandybridge"), nothrow, used, nonnull)) addtl_attrs6(int*);
 
-// FIXME: Declaration of a non-overloadable function when more than one
-// FIXME: multiversion function declarations are present results in an
-// FIXME: assertion failure.
 int __attribute__((target("sse4.2"))) bad_overload1(void);
 int __attribute__((target("arch=sandybridge"))) bad_overload1(void);
-// expected-error@+2 {{at most one overload for a given name may lack the 'overloadable' attribute}}
-// expected-note@-2 {{previous unmarked overload of function is here}}
+// expected-error@+1 {{function declaration is missing 'target' attribute in a multiversioned function}}
 int bad_overload1(int);
 
 int bad_overload2(int);
 // expected-error@+2 {{conflicting types for 'bad_overload2'}}
 // expected-note@-2 {{previous declaration is here}}
 int __attribute__((target("sse4.2"))) bad_overload2(void);
 // expected-error@+2 {{conflicting types for 'bad_overload2'}}
 // expected-note@-5 {{previous declaration is here}}