[clang][Sema] Fix incomplete C mode incompatible ExtInfo/ExtProtoInfo conversion diagnostic #160477
Conversation
… conversion diagnostic To raise -Wincompatible-function-pointer-type for extended function type information conversions in C mode, checkPointerTypesForAssignment contained an invocation of IsFunctionConversion that is quite dubious: it verifies the conversion from LHS to RHS, while we are converting from RHS to LHS. Presumably, this was done because result and argument types don't have to match exactly in C mode (compatible types is sufficient), while IsFunctionConversion checks that an exact match in function types can be obtained by transforming extended function type information. As a result of the dubious invocation, the diagnostic for incompatible extended function type changes was incomplete and would illogically disappear when other changes to the function type were involved (this was already raised in 2019 but left unsolved). Newer additions of extended function type information such as "cfi_unchecked_callee" and function effect attributes, worked around the issue by introducing more complex checking logic that could open other holes. Here, we solve this longstanding issue by using IsFunctionConverions to verify the extended function type information conversion in the correct direction (from RHS to LHS), after first removing other differences in the function type (argument and result type mismatch, prototype vs. no prototype) to allow IsFunctionConversion to obtain an exact match. The complications for checking "cfi_unchecked_callee" and function effect attributes are removed. Fixes llvm#160474
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Bruno De Fraine (brunodf-snps) ChangesTo raise -Wincompatible-function-pointer-type for extended function type information conversions in C mode, checkPointerTypesForAssignment contained an invocation of IsFunctionConversion that is quite dubious: it verifies the conversion from LHS to RHS, while we are converting from RHS to LHS. Presumably, this was done because result and argument types don't have to match exactly in C mode (compatible types is sufficient), while IsFunctionConversion checks that an exact match in function types can be obtained by transforming extended function type information. As a result of the dubious invocation, the diagnostic for incompatible extended function type changes was incomplete and would illogically disappear when other changes to the function type were involved (this was already raised in 2019 but left unsolved). Newer additions of extended function type information such as "cfi_unchecked_callee" and function effect attributes, worked around the issue by introducing more complex checking logic that could open other holes. Here, we solve this longstanding issue by using IsFunctionConverions to verify the extended function type information conversion in the correct direction (from RHS to LHS), after first removing other differences in the function type (argument and result type mismatch, prototype vs. no prototype) to allow IsFunctionConversion to obtain an exact match. The complications for checking "cfi_unchecked_callee" and function effect attributes are removed. Fixes #160474 Full diff: https://github.com/llvm/llvm-project/pull/160477.diff 4 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d017d1f829015..503fab7190c40 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10208,15 +10208,12 @@ class Sema final : public SemaBase {
bool CStyle, bool &ObjCLifetimeConversion);
/// Determine whether the conversion from FromType to ToType is a valid
- /// conversion that strips "noexcept" or "noreturn" or "cfi_unchecked_callee"
- /// off the nested function type. This also checks if "cfi_unchecked_callee"
- /// was added to the function type. If "cfi_unchecked_callee" is added and
- /// `AddingCFIUncheckedCallee` is provided, it will be set to true. The same
- /// thing applies for `DiscardingCFIUncheckedCallee` if the attribute is
- /// discarded.
- bool IsFunctionConversion(QualType FromType, QualType ToType,
- bool *DiscardingCFIUncheckedCallee = nullptr,
- bool *AddingCFIUncheckedCallee = nullptr) const;
+ /// conversion of ExtInfo/ExtProtoInfo on the nested function type.
+ /// More precisely, this method checks whether FromType can be tranformed
+ /// into an exact match for ToType, by transforming its extended function
+ /// type information in legal manner (e.g. by strictly stripping "noreturn"
+ /// or "noexcept", or by stripping "noescape" for arguments).
+ bool IsFunctionConversion(QualType FromType, QualType ToType) const;
/// Same as `IsFunctionConversion`, but if this would return true, it sets
/// `ResultTy` to `ToType`.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 3b267c1b1693d..6cb55659581d2 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -9187,12 +9187,43 @@ static AssignConvertType checkPointerTypesForAssignment(Sema &S,
return AssignConvertType::IncompatibleFunctionPointer;
return AssignConvertType::IncompatiblePointer;
}
- bool DiscardingCFIUncheckedCallee, AddingCFIUncheckedCallee;
- if (!S.getLangOpts().CPlusPlus &&
- S.IsFunctionConversion(ltrans, rtrans, &DiscardingCFIUncheckedCallee,
- &AddingCFIUncheckedCallee)) {
- // Allow conversions between CFIUncheckedCallee-ness.
- if (!DiscardingCFIUncheckedCallee && !AddingCFIUncheckedCallee)
+ const auto *LFT = ltrans->getAs<FunctionType>();
+ const auto *RFT = rtrans->getAs<FunctionType>();
+ if (!S.getLangOpts().CPlusPlus && LFT && RFT) {
+ // The invocation of IsFunctionConversion below will try to transform rtrans
+ // to obtain an exact match for ltrans. This should not fail because of
+ // mismatches in result type and parameter types, they were already checked
+ // by typesAreCompatible above. So we will recreate rtrans using the result
+ // type and parameter types from ltrans, but keeping its
+ // ExtInfo/ExtProtoInfo.
+ const auto *LFPT = dyn_cast<FunctionProtoType>(LFT);
+ const auto *RFPT = dyn_cast<FunctionProtoType>(RFT);
+ if (LFPT && RFPT) {
+ rtrans = S.Context.getFunctionType(LFPT->getReturnType(),
+ LFPT->getParamTypes(),
+ RFPT->getExtProtoInfo());
+ } else if (LFPT) {
+ FunctionProtoType::ExtProtoInfo EPI;
+ EPI.ExtInfo = RFT->getExtInfo();
+ rtrans = S.Context.getFunctionType(LFPT->getReturnType(),
+ LFPT->getParamTypes(), EPI);
+ } else if (RFPT) {
+ // In this case, we want to retain rtrans as a FunctionProtoType, to keep
+ // all of its ExtProtoInfo. To enable an exact match, we recreate ltrans
+ // as a FunctionProtoType, using its own info but filling in the parameter
+ // types from rtrans.
+ FunctionProtoType::ExtProtoInfo EPI;
+ EPI.ExtInfo = LFT->getExtInfo();
+ ltrans = S.Context.getFunctionType(LFT->getReturnType(),
+ RFPT->getParamTypes(), EPI);
+ rtrans = S.Context.getFunctionType(
+ LFT->getReturnType(), RFPT->getParamTypes(), RFPT->getExtProtoInfo());
+ } else {
+ rtrans = S.Context.getFunctionNoProtoType(LFT->getReturnType(),
+ RFT->getExtInfo());
+ }
+ if (!S.Context.hasSameUnqualifiedType(rtrans, ltrans) &&
+ !S.IsFunctionConversion(rtrans, ltrans))
return AssignConvertType::IncompatibleFunctionPointer;
}
if (IsInvalidCmseNSCallConversion(S, ltrans, rtrans))
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index ea5c4265d736d..84b4b83e573e6 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1891,14 +1891,7 @@ bool Sema::TryFunctionConversion(QualType FromType, QualType ToType,
return Changed;
}
-bool Sema::IsFunctionConversion(QualType FromType, QualType ToType,
- bool *DiscardingCFIUncheckedCallee,
- bool *AddingCFIUncheckedCallee) const {
- if (DiscardingCFIUncheckedCallee)
- *DiscardingCFIUncheckedCallee = false;
- if (AddingCFIUncheckedCallee)
- *AddingCFIUncheckedCallee = false;
-
+bool Sema::IsFunctionConversion(QualType FromType, QualType ToType) const {
if (Context.hasSameUnqualifiedType(FromType, ToType))
return false;
@@ -1957,25 +1950,14 @@ bool Sema::IsFunctionConversion(QualType FromType, QualType ToType,
const auto *ToFPT = dyn_cast<FunctionProtoType>(ToFn);
if (FromFPT && ToFPT) {
- if (FromFPT->hasCFIUncheckedCallee() && !ToFPT->hasCFIUncheckedCallee()) {
- QualType NewTy = Context.getFunctionType(
- FromFPT->getReturnType(), FromFPT->getParamTypes(),
- FromFPT->getExtProtoInfo().withCFIUncheckedCallee(false));
- FromFPT = cast<FunctionProtoType>(NewTy.getTypePtr());
- FromFn = FromFPT;
- Changed = true;
- if (DiscardingCFIUncheckedCallee)
- *DiscardingCFIUncheckedCallee = true;
- } else if (!FromFPT->hasCFIUncheckedCallee() &&
- ToFPT->hasCFIUncheckedCallee()) {
+ if (FromFPT->hasCFIUncheckedCallee() != ToFPT->hasCFIUncheckedCallee()) {
QualType NewTy = Context.getFunctionType(
FromFPT->getReturnType(), FromFPT->getParamTypes(),
- FromFPT->getExtProtoInfo().withCFIUncheckedCallee(true));
+ FromFPT->getExtProtoInfo().withCFIUncheckedCallee(
+ ToFPT->hasCFIUncheckedCallee()));
FromFPT = cast<FunctionProtoType>(NewTy.getTypePtr());
FromFn = FromFPT;
Changed = true;
- if (AddingCFIUncheckedCallee)
- *AddingCFIUncheckedCallee = true;
}
}
@@ -2006,11 +1988,7 @@ bool Sema::IsFunctionConversion(QualType FromType, QualType ToType,
Changed = true;
}
- // For C, when called from checkPointerTypesForAssignment,
- // we need to not alter FromFn, or else even an innocuous cast
- // like dropping effects will fail. In C++ however we do want to
- // alter FromFn (because of the way PerformImplicitConversion works).
- if (Context.hasAnyFunctionEffects() && getLangOpts().CPlusPlus) {
+ if (Context.hasAnyFunctionEffects()) {
FromFPT = cast<FunctionProtoType>(FromFn); // in case FromFn changed above
// Transparently add/drop effects; here we are concerned with
diff --git a/clang/test/Sema/incompatible-function-pointer-types-extinfo.c b/clang/test/Sema/incompatible-function-pointer-types-extinfo.c
new file mode 100644
index 0000000000000..d82313df172f0
--- /dev/null
+++ b/clang/test/Sema/incompatible-function-pointer-types-extinfo.c
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -fsyntax-only %s -std=c99 -verify=expected
+// RUN: %clang_cc1 -fsyntax-only %s -std=c11 -verify=expected
+// RUN: %clang_cc1 -fsyntax-only %s -std=c23 -verify=expected,proto
+
+// This verifies that -Wincompatible-function-pointer-type diagnostics for
+// extended function type information are consistent, also in case of other
+// allowed funcion type difference in C.
+//
+// Test case adapted from issue #160474.
+
+enum E { A = -1, B };
+
+// Case 1: assignment adding noreturn
+
+int f1 (int);
+int (*fp1a)(int) __attribute__((noreturn)) = &f1; // expected-error {{incompatible function pointer types}}
+enum E (*fp1b)(int) __attribute__((noreturn)) = &f1; // expected-error {{incompatible function pointer types}}
+int (*fp1c)() __attribute__((noreturn)) = &f1; // expected-error {{incompatible function pointer types}}
+
+// Case 2: assignment adding noescape on arg
+
+int f2 (int* ) __attribute__((noreturn));
+int (*fp2a)(int* __attribute__((noescape))) __attribute__((noreturn)) = &f2; // expected-error {{incompatible function pointer types}}
+int (*fp2b)(int* __attribute__((noescape))) = &f2; // expected-error {{incompatible function pointer types}}
+
+// Case 3: assignment adding cfi_unchecked_callee
+
+int f3 (int* );
+int (*fp3a)(int* ) __attribute__((noreturn )) = &f3; // expected-error {{incompatible function pointer types}}
+int (*fp3b)(int* __attribute__((noescape))) = &f3; // expected-error {{incompatible function pointer types}}
+int (*fp3c)(int* ) __attribute__((noreturn,cfi_unchecked_callee)) = &f3; // expected-error {{incompatible function pointer types}}
+int (*fp3d)(int* __attribute__((noescape))) __attribute__(( cfi_unchecked_callee)) = &f3; // expected-error {{incompatible function pointer types}}
+int (*fp3e)(int* __attribute__((noescape))) __attribute__((noreturn,cfi_unchecked_callee)) = &f3; // expected-error {{incompatible function pointer types}}
+
+// Case 4: assignment to function with no prototype
+
+int f4 (int);
+int (*fp4a)(int) = &f4;
+int (*fp4b)() = &f4; // proto-error {{incompatible function pointer types}}
|
|
Haven’t looked at this in detail yet, but this reminds me of #85415, where we noticed some weirdness around this in sema but we never got around to investigating it. |
There was a problem hiding this comment.
I’m feeling somewhat conflicted about this approach: on the one hand it’s better than what we were doing before, but on the other rebuilding a bunch of types to circumvent our own analysis... a bit weird. I wonder if we could instead extract the parts of IsFunctionConversion() that we care about here into a separate function.
|
Adding some more reviewers for opinions |
This is definitely the same root cause. |
I agree that rebuilding the types is somewhat complex to reuse the logic from |
- Don't transform both ltrans and rtrans when one transform could suffice. - Further simplify unneeded code checks that were added to work around the invalid invocation. - Fix formatting in test case.
The point of This organization is all or nothing and prevents to extract part of I really doubt if this alternate approach is the way to go. All in all, I think what I'm proposing here, to rebuild types in
Gentle ping, and |
|
ping |
clang/include/clang/Sema/Sema.h
Outdated
| bool *DiscardingCFIUncheckedCallee = nullptr, | ||
| bool *AddingCFIUncheckedCallee = nullptr) const; | ||
| /// conversion of ExtInfo/ExtProtoInfo on the nested function type. | ||
| /// More precisely, this method checks whether FromType can be tranformed |
There was a problem hiding this comment.
| /// More precisely, this method checks whether FromType can be tranformed | |
| /// More precisely, this method checks whether FromType can be transformed |
There was a problem hiding this comment.
Typo corrected.
- Fix typo in comment - Add release notes entry
Added. |
To raise -Wincompatible-function-pointer-type for extended function type information conversions in C mode, checkPointerTypesForAssignment contained an invocation of IsFunctionConversion that is quite dubious: it verifies the conversion from LHS to RHS, while we are converting from RHS to LHS. Presumably, this was done because result and argument types don't have to match exactly in C mode (compatible types is sufficient), while IsFunctionConversion checks that an exact match in function types can be obtained by transforming extended function type information. As a result of the dubious invocation, the diagnostic for incompatible extended function type changes was incomplete and would illogically disappear when other changes to the function type were involved (this was already raised in 2019 but left unsolved). Newer additions of extended function type information such as "cfi_unchecked_callee", ARM CMSE attributes, ARM SME attributes, and function effect attributes, worked around the issue by introducing either holes or additional checks which complicates the logic.
Here, we solve this longstanding issue by using IsFunctionConversion to verify the extended function type information conversion in the correct direction (from RHS to LHS), after first removing other differences in the function type (argument and result type mismatch, prototype vs. no prototype) to allow IsFunctionConversion to obtain an exact match. The complications introduced for checking newer attributes are removed.
Fixes #41465
Fixes #85415