-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Fix crash with modules and constexpr destructor #69076
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15754,10 +15754,22 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx, | |
LValue LVal; | ||
LVal.set(VD); | ||
|
||
if (!EvaluateInPlace(Value, Info, LVal, this, | ||
/*AllowNonLiteralTypes=*/true) || | ||
EStatus.HasSideEffects) | ||
return false; | ||
{ | ||
// C++23 [intro.execution]/p5 | ||
// A full-expression is ... an init-declarator ([dcl.decl]) or a | ||
// mem-initializer. | ||
// So we need to make sure temporary objects are destroyed after having | ||
// evaluated the expression (per C++23 [class.temporary]/p4). | ||
// | ||
// FIXME: Otherwise this may break test/Modules/pr68702.cpp because the | ||
// serialization code calls ParmVarDecl::getDefaultArg() which strips the | ||
// outermost FullExpr, such as ExprWithCleanups. | ||
FullExpressionRAII Scope(Info); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to add a comment here similar to the one in CC @cor3ntin for second look |
||
if (!EvaluateInPlace(Value, Info, LVal, this, | ||
/*AllowNonLiteralTypes=*/true) || | ||
EStatus.HasSideEffects) | ||
return false; | ||
} | ||
|
||
// At this point, any lifetime-extended temporaries are completely | ||
// initialized. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
// RUN: rm -rf %t | ||
// RUN: mkdir %t | ||
// RUN: split-file %s %t | ||
|
||
// RUN: %clang_cc1 -std=c++20 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t %t/main.cpp -o %t/main.o | ||
|
||
//--- V.h | ||
#ifndef V_H | ||
#define V_H | ||
|
||
class A { | ||
public: | ||
constexpr A() { } | ||
constexpr ~A() { } | ||
}; | ||
|
||
template <typename T> | ||
class V { | ||
public: | ||
V() = default; | ||
|
||
constexpr V(int n, const A& a = A()) {} | ||
}; | ||
|
||
#endif | ||
|
||
//--- inst1.h | ||
#include "V.h" | ||
|
||
static void inst1() { | ||
V<int> v; | ||
} | ||
|
||
//--- inst2.h | ||
#include "V.h" | ||
|
||
static void inst2() { | ||
V<int> v(100); | ||
} | ||
|
||
//--- module.modulemap | ||
module "M" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that the new standard nowadays? Can we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (not expressing any strong opinion on this patch in particular, but it'd be great if the pragmas were more used (but also, they're currently not perfect - my understanding is that |
||
export * | ||
module "V.h" { | ||
export * | ||
header "V.h" | ||
} | ||
module "inst1.h" { | ||
export * | ||
header "inst1.h" | ||
} | ||
} | ||
|
||
module "inst2.h" { | ||
export * | ||
header "inst2.h" | ||
} | ||
|
||
//--- main.cpp | ||
#include "V.h" | ||
#include "inst2.h" | ||
|
||
static void m() { | ||
static V<int> v(100); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the reason why this is related to modules. I feel the analysis is valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.