Page MenuHomePhabricator

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

Authored by mattd on Nov 28 2018, 5:08 PM.

Details

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.

Diff Detail

Event Timeline

mattd created this revision.Nov 28 2018, 5:08 PM

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

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 marked an inline comment as done.Nov 29 2018, 3:07 PM

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 updated this revision to Diff 176515.Dec 3 2018, 4:50 PM
  • 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 added inline comments.Dec 3 2018, 11:55 PM
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 marked an inline comment as done.Dec 4 2018, 9:48 AM
mattd added inline comments.
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 marked 2 inline comments as done.Dec 12 2018, 11:22 AM
mattd added inline comments.
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.

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 marked an inline comment as not done.Dec 18 2018, 8:05 AM

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 added a comment.Jan 10 2019, 3:25 PM

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