[sema] Warn of mangling change if function parameters are noexcept.

[mattd]

Summary

The flag: -Wc++17-compat-mangling was not emitting a warning diagnostic in all cases, specifically if a function has parameters that happen to have function pointers that have noexecept parameters. For example:

void fn(void (*)(void (*)() noexcept)){}

That example will produce different symbol names between c++17 and earlier dialects. I believe that clang should be emitting the compatibility warning in this case. This patch modifies the existing logic to recursively check the parameters of a function and warn if they too are noexcept.

This fixes PR39656.

[Rakete1111]

You're missing the case of the function return type of a parameter.

void f(void (*g())() noexcept); // no warning with trunk nor with your patch

[erik.pilkington]

It seems like this check isn't really doing enough to catch this break in full generality. For instance, the mangling of the following changes from C++14 to 17, but isn't diagnosed here:

template <class T> struct something {};
void f(something<void (*)() noexcept>) {}

The right thing to do is probably to use a more beefy traversal here, so I guess this is a step in the right direction.

lib/Sema/SemaDecl.cpp

10267

Can you just make this lambda a static function? Recursive lambdas are a bit crazy :)

[mattd]

Thanks for the reviews and added example. I'll update the logic to account for the return type, and add @Rakete1111 's example to the existing test.

lib/Sema/SemaDecl.cpp

10267

Not a problem! Actually, in my original fix I was hacking on, I moved the lambda into its own static function. But after looking at the patch, the least invasive thing, to keep the patch as small as possible, was just to make the lambda recursive... and part of me really liked the idea of a recursive lambda. I'm working on an update, and will move the lambda to a static function.

[mattd]

• Move the HasNoexcept lambda to its own static function.
• Added an additional check in HasNoexcept to recursively check return values.
• Modified the parameter check in Sema::CheckFunctionDeclaration to use llvm::any_of, all we need to know is if any of the parameters might have a mangling change.
• Updated the test to include the example provided by @Rakete1111.

[Rakete1111]

lib/Sema/SemaDecl.cpp

9933

You don't need this lambda.

10266

What you're doing here is a subset of what hasNoexcept is doing for function types. Do you think it would make sense to use !FPT->isNothrow() && hasNoexcept(NewFW->getType()) or something like that?

10268

Same, that lambda is unnecessary.

[mattd]

lib/Sema/SemaDecl.cpp

10266

Thanks for the comments @Rakete1111.

I do see the similarity and was trying to implement a solution to take advantage of that. However, I was running into trouble on assert/debug builds. llvm_unreachable, called in FunctionProtoType::canThrow, will trigger on FunctionProtoType instances that have not yet had their exception-spec type evaluated. canThrow is called on behalf of isNothrow. Some of the test cases will fail in assert/debug builds if we place a call to to FPT->isNothrow here.

The original code avoided this assertion by not calling isNothrow on FPT here, but instead just evaluating the return and parameter types of FPT. I made this patch similar to what was already in place, but with the more thorough check in hasNoexcept. I'd be pleased to remove this duplication/similarity if I can avoid this assertion.

[mattd]

lib/Sema/SemaDecl.cpp

10266

I just got back into looking at this issue. Given that I was unable to implement !FPT->isNoThrow() && hasNoexcept(NewFW->getType()) without triggering an assertion (discussed in my reply immediately above), is there anything we should be doing differently here? The change, as it sits now (with the duplication), seems to work fine.

[Rakete1111]

Apart from the comments I think your patch LGTM, but I'll let someone else hav the final say.

lib/Sema/SemaDecl.cpp

10266

I think it's fine.

10268

You marked this comment as done but the lambda is still there :).

[mattd]

In D55039#1334195, @Rakete1111 wrote:

Apart from the comments I think your patch LGTM, but I'll let someone else hav the final say.

Thanks for taking a peek!

[mattd]

*Ping* to see if anyone else has any feedback towards this patch.

lib/Sema/SemaDecl.cpp

static bool CheckMultiVersionFunction(Sema &S, FunctionDecl *NewFD,
   // At this point, we have a multiversion function decl (in OldFD) AND an
   // appropriate attribute in the current function decl.  Resolve that these are
   // still compatible with previous declarations.
   return CheckMultiVersionAdditionalDecl(
       S, OldFD, NewFD, MVType, NewTA, NewCPUDisp, NewCPUSpec, Redeclaration,
       OldDecl, MergeTypeWithPrevious, Previous);
 }
 
+/// Returns true if the type T is a function type that contains a noexcept specifier.
+/// This check recursively checks the params and return types of T if T happens
+/// to be a function.
+static bool hasNoexcept(QualType T) {
+  // Strip off declarator chunks that could be between us and a function
+  // type. We don't need to look far, exception specifications are very
+  // restricted prior to C++17.
+  if (auto *RT = T->getAs<ReferenceType>())
+    T = RT->getPointeeType();
+  else if (T->isAnyPointerType())
+    T = T->getPointeeType();
+  else if (auto *MPT = T->getAs<MemberPointerType>())
+    T = MPT->getPointeeType();
+  if (auto *FPT = T->getAs<FunctionProtoType>()) {
+    if (FPT->isNothrow() || hasNoexcept(FPT->getReturnType()) ||
+        llvm::any_of(FPT->param_types(),
+                     [](QualType Q) { return hasNoexcept(Q); }))
+      return true;
+  }
+  return false;
+}
+
 /// Perform semantic checking of a new function declaration.
 ///
 /// Performs semantic analysis of the new function declaration
 /// NewFD. This routine performs all semantic checking that does not
 /// require the actual declarator involved in the declaration, and is
 /// used both for the declaration of functions as they are parsed
 /// (called via ActOnDeclarator) and for the declaration of functions
 /// that have been instantiated via C++ template instantiation (called
if (getLangOpts().CPlusPlus) {
     //   [is] part of the function type
     //
     // This results in an ABI break between C++14 and C++17 for functions whose
     // declared type includes an exception-specification in a parameter or
     // return type. (Exception specifications on the function itself are OK in
     // most cases, and exception specifications are not permitted in most other
     // contexts where they could make it into a mangling.)
     if (!getLangOpts().CPlusPlus17 && !NewFD->getPrimaryTemplate()) {
-      auto HasNoexcept = [&](QualType T) -> bool {
-        // Strip off declarator chunks that could be between us and a function
-        // type. We don't need to look far, exception specifications are very
-        // restricted prior to C++17.
-        if (auto *RT = T->getAs<ReferenceType>())
-          T = RT->getPointeeType();
-        else if (T->isAnyPointerType())
-          T = T->getPointeeType();
-        else if (auto *MPT = T->getAs<MemberPointerType>())
-          T = MPT->getPointeeType();
-        if (auto *FPT = T->getAs<FunctionProtoType>())
-          if (FPT->isNothrow())
-            return true;
-        return false;
-      };
-
       auto *FPT = NewFD->getType()->castAs<FunctionProtoType>();
-      bool AnyNoexcept = HasNoexcept(FPT->getReturnType());
-      for (QualType T : FPT->param_types())
-        AnyNoexcept |= HasNoexcept(T);
-      if (AnyNoexcept)
+      if (hasNoexcept(FPT->getReturnType()) ||
+          llvm::any_of(FPT->param_types(),
+                       [](QualType Q) { return hasNoexcept(Q); })) {
         Diag(NewFD->getLocation(),
              diag::warn_cxx17_compat_exception_spec_in_signature)
             << NewFD;
       }
+    }
 
     if (!Redeclaration && LangOpts.CUDA)
       checkCUDATargetOverload(NewFD, Previous);
   }
   return Redeclaration;
 }
 
 void Sema::CheckMain(FunctionDecl* FD, const DeclSpec& DS) {

test/CXX/except/except.spec/p2-places.cpp

namespace noex {
   void (*fp)() noexcept(false);
 
   void g(void pfa() noexcept(false));
 
   typedef int (*pf)() noexcept(false); // expected-error {{specifications are not allowed in typedefs}}
 
   void (*h())() noexcept(false);
 
-  void (*i() noexcept(false))(void (*)() noexcept(true)) noexcept(false);
+  void (*i() noexcept(false))(void (*)() noexcept(true)) noexcept(false); // expected-warning {{mangled name of 'i' will change in C++17 due to non-throwing exception specification in function signature}}
 
   void (**k)(void pfa() noexcept(false)); // no-error
 
   void (**j)() noexcept(false); // expected-error {{not allowed beyond a single}}
 
   void (**(*h())())() noexcept(false); // expected-error {{not allowed beyond a single}}
 }

test/SemaCXX/cxx1z-noexcept-function-type.cpp

namespace CompatWarning {
   // These cases take an ABI break in C++17 because their parameter / return types change.
   void f1(void p() noexcept);
   void f2(void (*p)() noexcept(true));
   void f3(void (&p)() throw());
   void f4(void (X::*p)() throw());
   auto f5() -> void (*)() throw();
   auto f6() -> void (&)() throw();
   auto f7() -> void (X::*)() throw();
+  void f8(int, void (*)(int, void (*)() noexcept));
+  void f9(void (*g())() noexcept);
 #if __cplusplus <= 201402L && !defined(NO_COMPAT_MANGLING)
-  // expected-warning@-8 {{mangled name of 'f1' will change in C++17 due to non-throwing exception specification in function signature}}
-  // expected-warning@-8 {{mangled name of 'f2' will change in C++17 due to non-throwing exception specification in function signature}}
-  // expected-warning@-8 {{mangled name of 'f3' will change in C++17 due to non-throwing exception specification in function signature}}
-  // expected-warning@-8 {{mangled name of 'f4' will change in C++17 due to non-throwing exception specification in function signature}}
-  // expected-warning@-8 {{mangled name of 'f5' will change in C++17 due to non-throwing exception specification in function signature}}
-  // expected-warning@-8 {{mangled name of 'f6' will change in C++17 due to non-throwing exception specification in function signature}}
-  // expected-warning@-8 {{mangled name of 'f7' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f1' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f2' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f3' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f4' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f5' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f6' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f7' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f8' will change in C++17 due to non-throwing exception specification in function signature}}
+  // expected-warning@-10 {{mangled name of 'f9' will change in C++17 due to non-throwing exception specification in function signature}}
 #endif
 
   // An instantiation-dependent exception specification needs to be mangled in
   // all language modes, since it participates in SFINAE.
   template<typename T> void g(void() throw(T)); // expected-note {{substitution failure}}
   template<typename T> void g(...) = delete; // expected-note {{deleted}}
   void test_g() { g<void>(nullptr); } // expected-error {{deleted}}