[clang][Sema] Diagnose exceptions only in non-dependent context in discarded try/catch/throw blocks
#139859
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Rajveer Singh Bharadwaj (Rajveer100) ChangesResolves #138939 When enabling Full diff: https://github.com/llvm/llvm-project/pull/139859.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index b2a982e953012..16a4da40eec17 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -854,7 +854,8 @@ ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
// Don't report an error if 'throw' is used in system headers or in an OpenMP
// target region compiled for a GPU architecture.
if (!IsOpenMPGPUTarget && !getLangOpts().CXXExceptions &&
- !getSourceManager().isInSystemHeader(OpLoc) && !getLangOpts().CUDA) {
+ !getSourceManager().isInSystemHeader(OpLoc) && !getLangOpts().CUDA &&
+ !CurContext->isDependentContext()) {
// Delay error emission for the OpenMP device code.
targetDiag(OpLoc, diag::err_exceptions_disabled) << "throw";
}
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index e8c1f8490342a..2a0c8b017e2e0 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -4305,7 +4305,8 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock,
// Don't report an error if 'try' is used in system headers or in an OpenMP
// target region compiled for a GPU architecture.
if (!IsOpenMPGPUTarget && !getLangOpts().CXXExceptions &&
- !getSourceManager().isInSystemHeader(TryLoc) && !getLangOpts().CUDA) {
+ !getSourceManager().isInSystemHeader(TryLoc) && !getLangOpts().CUDA &&
+ !CurContext->isDependentContext()) {
// Delay error emission for the OpenMP device code.
targetDiag(TryLoc, diag::err_exceptions_disabled) << "try";
}
diff --git a/clang/test/SemaCXX/no-exceptions.cpp b/clang/test/SemaCXX/no-exceptions.cpp
index 097123d3fe523..665d43e260601 100644
--- a/clang/test/SemaCXX/no-exceptions.cpp
+++ b/clang/test/SemaCXX/no-exceptions.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
// Various tests for -fno-exceptions
@@ -32,3 +32,28 @@ void g() {
}
}
+
+namespace test2 {
+template <auto enable> void foo(auto &&Fnc) {
+ if constexpr (enable)
+ try {
+ Fnc();
+ } catch (...) {
+ }
+ else
+ Fnc();
+}
+
+void bar1() {
+ foo<false>([] {});
+}
+
+template <typename T> void foo() {
+ try {
+ } catch (...) {
+ }
+ throw 1;
+}
+void bar2() { foo<int>(); }
+
+}
|
|
@erichkeane template <typename T> void foo() {
try {
} catch (...) {
}
throw 1;
}
void bar2() { foo<int>(); } |
|
@Rajveer100 I think instead of looking at the context, we should check if the expression ( |
I don't think we can just check the expression, the 'throw' itself could be completely fine. Consider: The expression is NOT dependent, but we don't wanna diagnose anyway unless it is instantiated.
What we will have to do likely is move the checking for this to some piece of common code that is used by TreeTransform for these two. I haven't looked at a good place, but you'll probably see Also note, even in THOSE cases we have to properly check the decl context, since it could be a partial specialization/instantiation, so please make sure those are tested as well! |
Checking the expression causes a crash: Details
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0. Program arguments: /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang -cc1 -internal-isystem /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/lib/clang/21/include -nostdsysteminc -fsyntax-only -verify -std=c++20 /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/clang/test/SemaCXX/no-exceptions.cpp
1. /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/clang/test/SemaCXX/no-exceptions.cpp:24:8: current parser token ';'
2. /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/clang/test/SemaCXX/no-exceptions.cpp:22:1: parsing namespace 'test1'
3. /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/clang/test/SemaCXX/no-exceptions.cpp:23:10: parsing function body 'test1::f'
4. /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/clang/test/SemaCXX/no-exceptions.cpp:23:10: in compound statement ('{}')
#0 0x000000010954d198 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10538d198)
#1 0x000000010954d71c PrintStackTraceSignalHandler(void*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10538d71c)
#2 0x000000010954b584 llvm::sys::RunSignalHandlers() (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10538b584)
#3 0x000000010954dfe8 SignalHandler(int, __siginfo*, void*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10538dfe8)
#4 0x00000001876af624 (/usr/lib/system/libsystem_platform.dylib+0x1804ab624)
#5 0x000000010a1ef9cc clang::Expr::isInstantiationDependent() const (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10602f9cc)
#6 0x000000010d507f54 clang::Sema::BuildCXXThrow(clang::SourceLocation, clang::Expr*, bool) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x109347f54)
#7 0x000000010d507df0 clang::Sema::ActOnCXXThrow(clang::Scope*, clang::SourceLocation, clang::Expr*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x109347df0)
#8 0x000000010cd0e954 clang::Parser::ParseThrowExpression() (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108b4e954)
#9 0x000000010ccec734 clang::Parser::ParseAssignmentExpression(clang::TypeCastState) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108b2c734)
#10 0x000000010ccec620 clang::Parser::ParseExpression(clang::TypeCastState) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108b2c620)
#11 0x000000010cd88a2c clang::Parser::ParseExprStatement(clang::Parser::ParsedStmtContext) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bc8a2c)
#12 0x000000010cd86d74 clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 24u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&, clang::ParsedAttributes&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bc6d74)
#13 0x000000010cd865e0 clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 24u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bc65e0)
#14 0x000000010cd8f1f8 clang::Parser::ParseCompoundStatementBody(bool) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bcf1f8)
#15 0x000000010cd90914 clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bd0914)
#16 0x000000010cdb078c clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bf078c)
#17 0x000000010cc8d06c clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::Parser::ParsedTemplateInfo&, clang::SourceLocation*, clang::Parser::ForRangeInit*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108acd06c)
#18 0x000000010cdaf884 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bef884)
#19 0x000000010cdaee10 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108beee10)
#20 0x000000010cdae00c clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bee00c)
#21 0x000000010ccbee98 clang::Parser::ParseInnerNamespace(llvm::SmallVector<clang::Parser::InnerNamespaceInfo, 4u> const&, unsigned int, clang::SourceLocation&, clang::ParsedAttributes&, clang::BalancedDelimiterTracker&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108afee98)
#22 0x000000010ccbe458 clang::Parser::ParseNamespace(clang::DeclaratorContext, clang::SourceLocation&, clang::SourceLocation) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108afe458)
#23 0x000000010cc8baf0 clang::Parser::ParseDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, clang::SourceLocation*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108acbaf0)
#24 0x000000010cdadaf8 clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bedaf8)
#25 0x000000010cdac010 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108bec010)
#26 0x000000010cc70f38 clang::ParseAST(clang::Sema&, bool, bool) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x108ab0f38)
#27 0x000000010b0dd6a4 clang::ASTFrontendAction::ExecuteAction() (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x106f1d6a4)
#28 0x000000010b0dcf38 clang::FrontendAction::Execute() (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x106f1cf38)
#29 0x000000010aff394c clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x106e3394c)
#30 0x000000010b21832c clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10705832c)
#31 0x00000001041d1b34 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x100011b34)
#32 0x00000001041c3700 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x100003700)
#33 0x00000001041c2424 clang_main(int, char**, llvm::ToolContext const&) (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x100002424)
#34 0x00000001041fcf0c main (/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang-21+0x10003cf0c)
#35 0x00000001872d6b4c
/Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/tools/clang/test/SemaCXX/Output/no-exceptions.cpp.script: line 1: 31304 Segmentation fault: 11 /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/bin/clang -cc1 -internal-isystem /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/build/lib/clang/21/include -nostdsysteminc -fsyntax-only -verify -std=c++20 /Users/rajveersingh/GitHub-OpenSource/llvm-project/llvm-project/clang/test/SemaCXX/no-exceptions.cpp
--
********************
********************
Failed Tests (1):
Clang :: SemaCXX/no-exceptions.cpp
Testing Time: 7.40s
Total Discovered Tests: 1
Failed: 1 (100.00%)
I will have a look. |
Rajveer100
force-pushed
the
try-throw-discarded-no-exceptions
branch
from
552e394 to
46a9b01
Compare
|
Moving the checks does work for these limited cases, but now that we don't have them there, one of the old tests now doesn't show up the diagnosis: void f() {
throw;
}
void g() {
try {
f();
} catch (...) {
}
}Since there is a separate call: return Actions.ActOnCXXTryBlock(TryLoc, TryBlock.get(), Handlers);and debugging under lldb, breakpoints don't reach (i.e exit early) the transform calls. |
Right, 'transform' only happens during template instantiation. So we need to find 'some place' (that is a seperate function) to do these diagnostics. I might suggest doing a new one, since there doesn't seem to be a sensible place to do so otherwise... perhaps something like: NOTE I had that return |
Naively, I think GCC's behavior seems to be "if the code is outside of a discarded statement, then diagnose exception constructs, otherwise do not diagnose exception constructs but still validate that they're otherwise correct" which... is a choice: https://godbolt.org/z/36x5qM4dG GCC documents this as a codegen option so it kind of makes sense from that perspective: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options but then they diagnose even when codegen is not enabled, which is confusing: https://godbolt.org/z/fWfzb94e8 Clang's behavior is: "yell, yell loudly, yell as often as you can, don't ever stop yelling", which is... a worse choice: https://godbolt.org/z/75Y3dMr87 Clang documents this as "enabling C++ exceptions" which is completely vague and unhelpful: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fcxx-exceptions Directly below, we use the same phrasing for What are the use cases for disabling exceptions but still allowing the constructs in source? e.g., do we want to start disabling the keywords entirely, and thus they won't appear in the AST? Or do we want them to be a codegen-only option, at which point they do appear in the AST and we only diagnose at CodeGen time (which is also inconsistent with GCC)? Or do we want to do whatever GCC does? (Note, Clang has -fcxx-exceptions and GCC does not, so we're already somewhat inconsistent between the two compilers.) |
Yep, thats the question :) I think the 1st one (ignoring the construct entirely/stop trying to parse it at all/etc) is a worse experience, and inconsistent with history. AND I wouldn't want people to have a C++ code dialect where they can use 'try' as a variable name. I don't think we should diagnose at CODEGEN time. I'm suggesting diagnosing them at 'non-dependent/non-discarded' time, but putting them in the AST anyway. This will inhibit codegen, but would mean tooling would be able to see them anyway. WHICH they'll have to do for dependent contexts anyway with the change. |
|
In C++26, exceptions are allowed in a constant evaluated context, which is used extensively by reflection and maybe P2300. I also think allowing Here is an interesting scenario constexpr void g() {
throw 42;
}
constexpr int f() {
try {g(); } catch {}
return 0;
}
static_assert(f() == 0); // the call to g() from here should be allowed
int main() {
return f(); // but not here if we want to disallow exceptions
}
So, now that I think about it, doing it at code gen is the only solution post C++26 |
Good point. :-/
[citation needed] You already cannot safely link code which disables exceptions into code which enables exceptions due to ABI issues, so I'm really not certain what harm would come from letting users use these identifiers themselves if they elect to ignore the language feature.
I disagree. If the user disabled exceptions, why would they expect them to work in constant expressions? |
|
I feel like this issue is a little too niche to be adding such complexity, maybe just covering those specific use cases before codegen might be the way to go? |
I think disabling the keywords entirely is a large ask (requires an RFC at a minimum) and so we don't need to go down that route right now. I think Since we currently validate that the exception constructs are semantically valid, I think we may as well retain them in the AST for now. It's how we currently behave: https://godbolt.org/z/xPbEPWTK8 so it would be consistent to do so in discarded statements too. |
I would be against said RFC. If you asked me in 1996/whatever, I could be talked into it. But we're 30+ years of this behavior.
I'm coming around on this one, I think I agree. -fno-exceptions means
|
Doesn't stop WG21 from changing the behavior 30 years later. ;-) My biggest concern is actually that C++ is changing exception handling such that we can no longer hand-wave what this flag means. Does it mean exceptions are enabled or not? If exceptions are not enabled, why are we requiring their use to be correct in source? And why do we want this flag to behave differently from other dialect flags like I think the important question is: what breaks if we were to do this? As best I can tell, nothing. Existing code using the flag continues to behave exactly as it did before, same with existing code not using the flag. The only issue boils down to users writing new code and trying to combine it with other TUs that are not compiled with the flag; but that's invalid anyway for ABI reasons, so I'm not seeing where the concerns are. |
I don't want to change the behavior that drastically. Folks are USED to this, and we should just stick to it. Putting them back into 'this could be an identifier' is a bit of extra FE headache at the cost of being 'different' on this than everyone else. IMO its not worth the effort nor difference. We should just stick to our current behavior, except allow discarded/uninstantiated statements. |
|
So @Rajveer100 : I think I bottomed out with Aaron. I think the solution of 'create a function to diagnose these, but only if not in a dependent We can discuss the REST of the proposals/ideas in the future if we get an RFC from someone. In addition, I think there is value to considering an additional flag that works with |
Agreed, thank you for the discussion! |
Yeah, I agree there should be some way to enable exceptions only during constant evaluation once we support that. |
Rajveer100
force-pushed
the
try-throw-discarded-no-exceptions
branch
from
46a9b01 to
214aa7f
Compare
|
I have made the changes, let me know if this is fair enough. |
Rajveer100
force-pushed
the
try-throw-discarded-no-exceptions
branch
2 times, most recently
from
f3201c1 to
136f1c0
Compare
Rajveer100
force-pushed
the
try-throw-discarded-no-exceptions
branch
from
136f1c0 to
2fc6e7c
Compare
|
Done. |
…scarded `try/catch/throw` blocks Resolves llvm#138939 When enabling `--fno-exceptions` flag, discarded statements containing `try/catch/throw` in an independent context can be avoided from being rejected.
Rajveer100
force-pushed
the
try-throw-discarded-no-exceptions
branch
from
2fc6e7c to
a385808
Compare
|
Thanks for merging, it would be nice if the members could help me with this: |
…scarded `try/catch/throw` blocks (llvm#139859) Resolves llvm#138939 When enabling `--fno-exceptions` flag, discarded statements containing `try/catch/throw` in an independent context can be avoided from being rejected.
Resolves #138939
When enabling
--fno-exceptionsflag, discarded statements containingtry/catch/throwin an independent context can be avoided from being rejected.