[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
This revision was automatically updated to reflect the committed changes.
Closed by commit rC352050: [ASTImporter] Fix inequality of functions with
different attributes (authored by martong, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D53699?vs=178060=183307#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53699/new/
https://reviews.llvm.org/D53699
Files:
lib/AST/ASTStructuralEquivalence.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -296,6 +296,32 @@
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 ,
+ 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 ,
QualType T1, QualType T2) {
@@ -539,7 +565,8 @@
if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
Function2->getReturnType()))
return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
+if (!IsStructurallyEquivalent(Context, Function1->getExtInfo(),
+ Function2->getExtInfo()))
return false;
break;
}
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,31 @@
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 {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -296,6 +296,32 @@
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 ,
+ 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 ,
QualType T1, QualType T2) {
@@ -539,7 +565,8 @@
if (!IsStructurallyEquivalent(Context,
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Thank you for adding the additional test. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong added a comment. Ping @shafik, I have addressed you comments, could you please take another look? If you don't have any objections, could you please approve? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong marked 4 inline comments as done.
martong added inline comments.
Comment at: lib/AST/ASTStructuralEquivalence.cpp:302
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits, e.g. the noreturn
+/// bit.
shafik wrote:
> This comment is confusing b/c it looks like the noreturn bits are the only
> one you are not checking.
Ok, I have removed that part of the comment.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:373
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {
shafik wrote:
> 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?
Ok, I have adder a few more tests and renamed the existing one.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53699/new/
https://reviews.llvm.org/D53699
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong updated this revision to Diff 178060.
martong marked 2 inline comments as done.
martong added a comment.
- Add more tests and simplify comment
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53699/new/
https://reviews.llvm.org/D53699
Files:
lib/AST/ASTStructuralEquivalence.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,31 @@
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 {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -297,6 +297,32 @@
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 ,
+ 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 ,
QualType T1, QualType T2) {
@@ -540,7 +566,8 @@
if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
Function2->getReturnType()))
return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
+if (!IsStructurallyEquivalent(Context, Function1->getExtInfo(),
+ Function2->getExtInfo()))
return false;
break;
}
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,31 @@
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 {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -297,6 +297,32 @@
return true;
}
+/// Determine structural equivalence based on the ExtInfo of functions. This
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
shafik added inline comments.
Comment at: lib/AST/ASTStructuralEquivalence.cpp:302
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits, e.g. the noreturn
+/// bit.
This comment is confusing b/c it looks like the noreturn bits are the only one
you are not checking.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:373
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {
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?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53699/new/
https://reviews.llvm.org/D53699
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong added a comment. Hi @a_sidorin , I have updated the patch as you suggested, to check the equivalence based on `ASTContext::mergeFunctionTypes()`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong updated this revision to Diff 175647.
martong added a comment.
- Use ExtInfo in structural equivalency
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53699/new/
https://reviews.llvm.org/D53699
Files:
lib/AST/ASTStructuralEquivalence.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,15 @@
EXPECT_FALSE(testStructuralMatch(t));
}
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {
+ auto t = makeNamedDecls(
+ "__attribute__((noreturn)) void foo();",
+ " void foo();",
+ Lang_C);
+ EXPECT_TRUE(testStructuralMatch(t));
+}
+
struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -297,6 +297,33 @@
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, e.g. the noreturn
+/// bit.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
+ 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 ,
QualType T1, QualType T2) {
@@ -540,7 +567,8 @@
if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
Function2->getReturnType()))
return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
+if (!IsStructurallyEquivalent(Context, Function1->getExtInfo(),
+ Function2->getExtInfo()))
return false;
break;
}
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,15 @@
EXPECT_FALSE(testStructuralMatch(t));
}
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {
+ auto t = makeNamedDecls(
+ "__attribute__((noreturn)) void foo();",
+ " void foo();",
+ Lang_C);
+ EXPECT_TRUE(testStructuralMatch(t));
+}
+
struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -297,6 +297,33 @@
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, e.g. the noreturn
+/// bit.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
+ 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 ,
QualType T1, QualType T2) {
@@ -540,7 +567,8 @@
if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong added a comment. Herald added a subscriber: gamesh411. 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. Repository: rC Clang https://reviews.llvm.org/D53699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
a_sidorin added a comment. 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? Repository: rC Clang https://reviews.llvm.org/D53699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes
martong created this revision.
martong added a reviewer: a_sidorin.
Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
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).
Repository:
rC Clang
https://reviews.llvm.org/D53699
Files:
lib/AST/ASTStructuralEquivalence.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,15 @@
EXPECT_FALSE(testStructuralMatch(t));
}
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {
+ auto t = makeNamedDecls(
+ "__attribute__((noreturn)) void foo();",
+ " void foo();",
+ Lang_C);
+ EXPECT_TRUE(testStructuralMatch(t));
+}
+
struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -540,8 +540,6 @@
if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
Function2->getReturnType()))
return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
- return false;
break;
}
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,15 @@
EXPECT_FALSE(testStructuralMatch(t));
}
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {
+ auto t = makeNamedDecls(
+ "__attribute__((noreturn)) void foo();",
+ " void foo();",
+ Lang_C);
+ EXPECT_TRUE(testStructuralMatch(t));
+}
+
struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
};
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -540,8 +540,6 @@
if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
Function2->getReturnType()))
return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
- return false;
break;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
