Skip to content

[Clang] SemaFunctionEffects: When verifying a function, ignore any conditional noexcept expression. #115342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dougsonos merged 3 commits into from
Nov 11, 2024
Merged

[Clang] SemaFunctionEffects: When verifying a function, ignore any conditional noexcept expression. #115342

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0b6093
[Clang] SemaFunctionEffects: When verifying a function, ignore any co…
Nov 5, 2024
02f1fc7
Review feedback: Check for no-prototype. Simplify test.
Nov 8, 2024
30dd821
Merge branch 'main' into nonblocking-expected-noexcept
dougsonos Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions clang/lib/Sema/SemaFunctionEffects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,7 @@ class Analyzer {
CallableInfo &CurrentCaller;
ViolationSite VSite;
const Expr *TrailingRequiresClause = nullptr;
const Expr *NoexceptExpr = nullptr;

FunctionBodyASTVisitor(Analyzer &Outer,
PendingFunctionAnalysis &CurrentFunction,
Expand All @@ -986,9 +987,22 @@ class Analyzer {
if (auto *Dtor = dyn_cast<CXXDestructorDecl>(CurrentCaller.CDecl))
followDestructor(dyn_cast<CXXRecordDecl>(Dtor->getParent()), Dtor);

if (auto *FD = dyn_cast<FunctionDecl>(CurrentCaller.CDecl))
if (auto *FD = dyn_cast<FunctionDecl>(CurrentCaller.CDecl)) {
TrailingRequiresClause = FD->getTrailingRequiresClause();

// Note that FD->getType->getAs<FunctionProtoType>() can yield a
// noexcept Expr which has been boiled down to a constant expression.
// Going through the TypeSourceInfo obtains the actual expression which
// will be traversed as part of the function -- unless we capture it
// here and have TraverseStmt skip it.
if (TypeSourceInfo *TSI = FD->getTypeSourceInfo()) {
FunctionProtoTypeLoc TL =
TSI->getTypeLoc().getAs<FunctionProtoTypeLoc>();
if (const FunctionProtoType *FPT = TL.getTypePtr())
NoexceptExpr = FPT->getNoexceptExpr();
}
}

// Do an AST traversal of the function/block body
TraverseDecl(const_cast<Decl *>(CurrentCaller.CDecl));
Copy link
Member

@Sirraide Sirraide Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I wonder if it wouldn’t be easier to just reimplement function traversal and not call TraverseDecl here rather than add more and more members that we then need to explicitly avoid visiting later on. From a cursory look, DEF_TRAVERSE_DECL, TraverseFunctionHelper (which is what’s used for most function-like things), and the traversal code for BlockDecls seem to have a lot of code paths we don’t really care about in this case.

Copy link
Member

@Sirraide Sirraide Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach should work too, though. So if you want to explore that approach separately (or I can also take a look at that since I’ve been refactoring AST visitors lately), then that’s also fine imo

Copy link
Contributor Author

@dougsonos dougsonos Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good thought. Looking at TraverseFunctionHelper:

  • the template arguments should be ignorable.
  • need to traverse the function's type because it contains the parameters -- e.g. I caught someone passing a vector by value instead of by reference and that showed up first as a call to the vector's destructor, located in the parameter list.
  • but the function's type is where the noexcept expression comes from.
  • can skip the trailing return clause.
  • need to traverse the constructor initializers.
  • need to traverse the body of course.

I'm not excited to tear this apart at the moment but maybe the next bug that comes up in this area can drive an improvement.

}
Expand Down Expand Up @@ -1269,7 +1283,8 @@ class Analyzer {
// We skip the traversal of lambdas (beyond their captures, see
// TraverseLambdaExpr below), so just caching this from our constructor
// should suffice.
if (Statement != TrailingRequiresClause)
// The exact same is true for a conditional `noexcept()` clause.
if (Statement != TrailingRequiresClause && Statement != NoexceptExpr)
return Base::TraverseStmt(Statement);
return true;
}
Expand Down
12 changes: 10 additions & 2 deletions clang/test/Sema/attr-nonblocking-constraints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ void nb26() [[clang::nonblocking]] {
abort_wrapper(); // no diagnostic
}

// --- Make sure we don't traverse a requires clause. ---
// --- Make sure we don't traverse requires and noexcept clauses. ---

// Apparently some requires clauses are able to be collapsed into a constant before the nonblocking
// analysis sees any function calls. This example (extracted from a real-world case where
Expand Down Expand Up @@ -420,19 +420,27 @@ class expected {
constexpr expected()
{}

// This is a deliberate corruption of the real implementation for simplicity.
constexpr expected(const expected&)
noexcept(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err>)
Copy link
Member

@Sirraide Sirraide Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like this would be a simpler, more self-contained test:

constexpr bool foo() [[clang::nonblocking(false)]] { return true; }
void f() noexcept(foo()) [[clang::nonblocking]] {}

This currently causes a warning on godbolt.

Copy link
Member

@Sirraide Sirraide Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of constant-evaluation, this currently causes warnings:

constexpr bool foo() [[clang::nonblocking]] { 
    if consteval {
        auto p = new int;
        delete p;
    }

    return true; 
}

I think simply ignoring the body of a consteval if (or the else clause if there is one of a !consteval if) should be enough; that doesn’t have to be in this pr tho.

requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err>)
= default;
};

void test() [[clang::nonblocking]]
{
expected<int, int> a;
auto b = a;
auto b = a; // Copy constructor.
}

} // namespace ExpectedTest

// Make sure that simple type traits don't cause violations.

void nb27() [[clang::nonblocking]] {
bool x = __is_constructible(int, const int&);
}

// --- nonblocking implies noexcept ---
#pragma clang diagnostic warning "-Wperf-constraint-implies-noexcept"

Expand Down
Loading