[clang][Sema] close IsStandardConversion hole when adding cfi_unchecked_callee

[brunodf-snps]

Commit b194cf1 changed this function for the case where attribute cfi_unchecked_callee is added in a function conversion. But this introduces a hole (issue #162798), and it seems the change was unnecessary: the preceding TryFunctionConversion will already allow adding the cfi_unchecked_callee attribute, and will update FromType if it succeeds. So we revert the changes to IsStandardConversion. We also remove the helper function AddingCFIUncheckedCallee which is no longer needed, and simplify the corresponding DiscardingCFIUncheckedCallee.

Fixes: #162798

[llvmbot]

@llvm/pr-subscribers-clang

Author: Bruno De Fraine (brunodf-snps)

Changes

Commit b194cf1 changed this function for the case where attribute cfi_unchecked_callee is added in a function conversion. But this introduces a hole (issue #162798), and it seems the change was unnecessary: the preceding TryFunctionConversion will already allow adding the cfi_unchecked_calleeattribute, and will updateFromTypeif it succeeds. So we revert the changes toIsStandardConversion. We also remove the helper function AddingCFIUncheckedCalleewhich is no longer needed, and simplify the correspondingDiscardingCFIUncheckedCallee`.

Fixes: #162798

---

Full diff: https://github.com/llvm/llvm-project/pull/164592.diff

4 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (-5)
• (modified) clang/lib/Sema/SemaChecking.cpp (+5-25)
• (modified) clang/lib/Sema/SemaOverload.cpp (+4-7)
• (modified) clang/test/Frontend/cfi-unchecked-callee-attribute.cpp (+1)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index cb21335ede075..85fdc3b286e46 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2685,11 +2685,6 @@ class Sema final : public SemaBase {
   /// function without this attribute.
   bool DiscardingCFIUncheckedCallee(QualType From, QualType To) const;
 
-  /// Returns true if `From` is a function or pointer to a function without the
-  /// `cfi_unchecked_callee` attribute but `To` is a function or pointer to
-  /// function with this attribute.
-  bool AddingCFIUncheckedCallee(QualType From, QualType To) const;
-
   /// This function calls Action when it determines that E designates a
   /// misaligned member due to the packed attribute. This is used to emit
   /// local diagnostics like in reference binding.
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 2990fd67cc53d..cfac54ffd9225 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -12351,14 +12351,9 @@ static void DiagnoseMixedUnicodeImplicitConversion(Sema &S, const Type *Source,
   }
 }
 
-enum CFIUncheckedCalleeChange {
-  None,
-  Adding,
-  Discarding,
-};
-
-static CFIUncheckedCalleeChange AdjustingCFIUncheckedCallee(QualType From,
-                                                            QualType To) {
+bool Sema::DiscardingCFIUncheckedCallee(QualType From, QualType To) const {
+  From = Context.getCanonicalType(From);
+  To = Context.getCanonicalType(To);
   QualType MaybePointee = From->getPointeeType();
   if (!MaybePointee.isNull() && MaybePointee->getAs<FunctionType>())
     From = MaybePointee;
@@ -12370,25 +12365,10 @@ static CFIUncheckedCalleeChange AdjustingCFIUncheckedCallee(QualType From,
     if (const auto *ToFn = To->getAs<FunctionType>()) {
       if (FromFn->getCFIUncheckedCalleeAttr() &&
           !ToFn->getCFIUncheckedCalleeAttr())
-        return Discarding;
-      if (!FromFn->getCFIUncheckedCalleeAttr() &&
-          ToFn->getCFIUncheckedCalleeAttr())
-        return Adding;
+        return true;
     }
   }
-  return None;
-}
-
-bool Sema::DiscardingCFIUncheckedCallee(QualType From, QualType To) const {
-  From = Context.getCanonicalType(From);
-  To = Context.getCanonicalType(To);
-  return ::AdjustingCFIUncheckedCallee(From, To) == Discarding;
-}
-
-bool Sema::AddingCFIUncheckedCallee(QualType From, QualType To) const {
-  From = Context.getCanonicalType(From);
-  To = Context.getCanonicalType(To);
-  return ::AdjustingCFIUncheckedCallee(From, To) == Adding;
+  return false;
 }
 
 void Sema::CheckImplicitConversion(Expr *E, QualType T, SourceLocation CC,
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 7da09e8b8e911..79dddfa1eaf92 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -2532,15 +2532,12 @@ static bool IsStandardConversion(Sema &S, Expr* From, QualType ToType,
 
   SCS.setToType(2, FromType);
 
-  // If we have not converted the argument type to the parameter type,
-  // this is a bad conversion sequence, unless we're resolving an overload in C.
-  //
-  // Permit conversions from a function without `cfi_unchecked_callee` to a
-  // function with `cfi_unchecked_callee`.
-  if (CanonFrom == CanonTo || S.AddingCFIUncheckedCallee(CanonFrom, CanonTo))
+  if (CanonFrom == CanonTo)
     return true;
 
-  if ((S.getLangOpts().CPlusPlus || !InOverloadResolution))
+  // If we have not converted the argument type to the parameter type,
+  // this is a bad conversion sequence, unless we're resolving an overload in C.
+  if (S.getLangOpts().CPlusPlus || !InOverloadResolution)
     return false;
 
   ExprResult ER = ExprResult{From};
diff --git a/clang/test/Frontend/cfi-unchecked-callee-attribute.cpp b/clang/test/Frontend/cfi-unchecked-callee-attribute.cpp
index 072f217ff7b19..a5a17dd5a4d82 100644
--- a/clang/test/Frontend/cfi-unchecked-callee-attribute.cpp
+++ b/clang/test/Frontend/cfi-unchecked-callee-attribute.cpp
@@ -9,6 +9,7 @@ void (*checked_ptr)(void) = unchecked;  // expected-warning{{implicit conversion
 void (CFI_UNCHECKED_CALLEE *unchecked_ptr)(void) = unchecked;
 void (CFI_UNCHECKED_CALLEE *from_normal)(void) = checked;
 void (CFI_UNCHECKED_CALLEE *c_no_function_decay)(void) = &unchecked;
+void (CFI_UNCHECKED_CALLEE __attribute__((noreturn)) *other_conflict)(void) = &checked; // expected-error{{cannot initialize a variable of type 'void (*)() __attribute__((noreturn)) __attribute__((cfi_unchecked_callee))' with an rvalue of type 'void (*)()'}}
 void (CFI_UNCHECKED_CALLEE *arr[10])(void);
 void (*cfi_elem)(void) = arr[1];  // expected-warning{{implicit conversion from 'void (*)() __attribute__((cfi_unchecked_callee))' to 'void (*)()' discards 'cfi_unchecked_callee' attribute}}
 void (CFI_UNCHECKED_CALLEE *cfi_unchecked_elem)(void) = arr[1];

[PiJoules]

Thanks for fixing this for me. At a glance this looks good. Lemme just apply this locally to see if this changes anything for our project.

[PiJoules]

LGTM thanks

[brunodf-snps]

Thanks for checking! I'll wait a day or two more to see if anyone else has remarks before merging this.