[ASTImporter] Fix inequality of functions with different attributes

[martong]

Summary

FunctionType::ExtInfo holds such properties of a function which are needed
mostly for code gen. We should not compare these bits when checking for
structural equivalency.
Checking ExtInfo caused false ODR errors during CTU analysis (of tmux).

[a_sidorin]

Hi Gabor,
Thank you for the patch. The reason for this change looks clear. However, I don't think omitting this comparison completely is what we want here. Instead, we can do a dance similar to ASTContext::mergeFunctionTypes() where all attributes but NoReturn are compared. What do you think?

[martong]

That's a good point. I agree, we should check some bits (calling convention bits) but not all (e.g. noreturn bit). I am going to create another patch which replaces this.

[martong]

• Use ExtInfo in structural equivalency

[martong]

Hi @a_sidorin ,

I have updated the patch as you suggested, to check the equivalence based on ASTContext::mergeFunctionTypes().

[a_sidorin]

LGTM, thanks!

[shafik]

lib/AST/ASTStructuralEquivalence.cpp

301

This comment is confusing b/c it looks like the noreturn bits are the only one you are not checking.

unittests/AST/StructuralEquivalenceTest.cpp

373

Can we get some more tests to be a little more thorough and can we also get a test where it is expected to to be false as well?

[martong]

lib/AST/ASTStructuralEquivalence.cpp

301

Ok, I have removed that part of the comment.

unittests/AST/StructuralEquivalenceTest.cpp

373

Ok, I have adder a few more tests and renamed the existing one.

[martong]

• Add more tests and simplify comment

[martong]

Ping @shafik, I have addressed you comments, could you please take another look? If you don't have any objections, could you please approve?

[shafik]

LGTM

Thank you for adding the additional test.

lib/AST/ASTStructuralEquivalence.cpp

static bool IsArrayStructurallyEquivalent(StructuralEquivalenceContext &Context,
   if (Array1->getSizeModifier() != Array2->getSizeModifier())
     return false;
   if (Array1->getIndexTypeQualifiers() != Array2->getIndexTypeQualifiers())
     return false;
 
   return true;
 }
 
+/// Determine structural equivalence based on the ExtInfo of functions. This
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     FunctionType::ExtInfo EI1,
+                                     FunctionType::ExtInfo EI2) {
+  // Compatible functions must have compatible calling conventions.
+  if (EI1.getCC() != EI2.getCC())
+    return false;
+
+  // Regparm is part of the calling convention.
+  if (EI1.getHasRegParm() != EI2.getHasRegParm())
+    return false;
+  if (EI1.getRegParm() != EI2.getRegParm())
+    return false;
+
+  if (EI1.getProducesResult() != EI2.getProducesResult())
+    return false;
+  if (EI1.getNoCallerSavedRegs() != EI2.getNoCallerSavedRegs())
+    return false;
+  if (EI1.getNoCfCheck() != EI2.getNoCfCheck())
+    return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two types.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      QualType T1, QualType T2) {
   if (T1.isNull() || T2.isNull())
     return T1.isNull() && T2.isNull();
 
   QualType OrigT1 = T1;
   QualType OrigT2 = T2;
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   }
 
   case Type::FunctionNoProto: {
     const auto *Function1 = cast<FunctionType>(T1);
     const auto *Function2 = cast<FunctionType>(T2);
     if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
                                   Function2->getReturnType()))
       return false;
-    if (Function1->getExtInfo() != Function2->getExtInfo())
+    if (!IsStructurallyEquivalent(Context, Function1->getExtInfo(),
+                                  Function2->getExtInfo()))
       return false;
     break;
   }
 
   case Type::UnresolvedUsing:
     if (!IsStructurallyEquivalent(Context,
                                   cast<UnresolvedUsingType>(T1)->getDecl(),
                                   cast<UnresolvedUsingType>(T2)->getDecl()))

unittests/AST/StructuralEquivalenceTest.cpp

 TEST_F(StructuralEquivalenceFunctionTest, NameInParenWithConst) {
   auto t = makeNamedDecls(
       "struct A { void (foo)() const; };",
       "struct A { void (foo)(); };",
       Lang_CXX11);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentNoreturnAttr) {
+  auto t = makeNamedDecls(
+      "__attribute__((noreturn)) void foo();",
+      "                          void foo();",
+      Lang_C);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceFunctionTest,
+    FunctionsWithDifferentCallingConventions) {
+  auto t = makeNamedDecls(
+      "__attribute__((fastcall)) void foo();",
+      "__attribute__((ms_abi))   void foo();",
+      Lang_C);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentSavedRegsAttr) {
+  auto t = makeNamedDecls(
+      "__attribute__((no_caller_saved_registers)) void foo();",
+      "                                           void foo();",
+      Lang_C);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
 };
 
 TEST_F(StructuralEquivalenceCXXMethodTest, Virtual) {
   auto t = makeDecls<CXXMethodDecl>(
       "struct X { void foo(); };",
       "struct X { virtual void foo(); };", Lang_CXX,
       cxxMethodDecl(hasName("foo")));