[Clang] [Sema] Ignore invalid multiversion function redeclarations

[Izaron]

Summary

If a redeclaration of a multiversion function is invalid,
it may be in a broken condition (for example, missing an important
attribute). We shouldn't analyze invalid redeclarations.

Fixes https://github.com/llvm/llvm-project/issues/57343

[Izaron]

How the bug was working on this example:

void foo() {}
[[gnu::target("default")]] void foo() {}
[[gnu::target("avx2")]] void foo() {}

1. Clang parses the definition of the foo function (line 1)
2. When parsing the second definition (line 2), Clang will delete TargetAttr in this function
3. Eventually Clang will mark both foos as invalid.
4. When parsing the third definition (line 3), in the CheckMultiVersionAdditionalDecl method Clang is trying to compare TargetAttr of the 2nd and the 3rd foo: link.
5. The CurTA variable is equal to nullptr (because we deleted the attribute in the second step), so there is undefined behaviour.

I suggest to stop looking to invalid declarations.

[tahonermann]

Thank you, this looks good to me!

[erichkeane]

makes sense to me, thanks!

clang/lib/Sema/SemaDecl.cpp

static bool CheckMultiVersionAdditionalDecl(
   }
 
   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.
+  // Next, check ALL non-invalid 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)
+    if (!CurFD || CurFD->isInvalidDecl())
       continue;
     if (MayNeedOverloadableChecks &&
         S.IsOverload(NewFD, CurFD, UseMemberUsingDeclRules))
       continue;
 
     switch (NewMVKind) {
     case MultiVersionKind::None:
       assert(OldMVKind == MultiVersionKind::TargetClones &&

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

 // expected-note@-2 {{previous definition is here}}
 int __attribute__((target("arch=sandybridge")))  redef(void) { return 2; }
 
 int __attribute__((target("default"))) redef2(void) { return 1;}
 // expected-error@+2 {{redefinition of 'redef2'}}
 // expected-note@-2 {{previous definition is here}}
 int __attribute__((target("default"))) redef2(void) { return 1;}
 
+int redef3(void) { return 1; }
+// expected-warning@+4 {{attribute declaration must precede definition}}
+// expected-note@-2 {{previous definition is here}}
+// expected-error@+2 {{redefinition of 'redef3'}}
+// expected-note@-4 {{previous definition is here}}
+int __attribute__((target("default"))) redef3(void) { return 1; }
+// allow this, since we don't complain about more than one redefinition
+int __attribute__((target("sse4.2"))) redef3(void) { return 1; }
+
 int __attribute__((target("sse4.2"))) mv_after_use(void) { return 1; }
 int use3(void) {
   return mv_after_use();
 }
 
 // expected-error@+1 {{function declaration cannot become a multiversioned function after first usage}}
 int __attribute__((target("arch=sandybridge")))  mv_after_use(void) { return 2; }