Skip to content

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

Merged
merged 3 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,8 @@ Miscellaneous Clang Crashes Fixed
- Fixed a crash when a lambda marked as ``static`` referenced a captured
variable in an expression.
`Issue 74608 <https://github.com/llvm/llvm-project/issues/74608>`_
- Fixed a crash with modules and a ``constexpr`` destructor.
`Issue 68702 <https://github.com/llvm/llvm-project/issues/68702>`_


OpenACC Specific Changes
Expand Down
20 changes: 16 additions & 4 deletions clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// evaluated the expression (per C++23 [class.temporary]/p4).
// 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.

I mean the reason why this is related to modules. I feel the analysis is valuable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

//
// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 EvaluateAsConstexpr(...) in this case it would be A full-expression is ... an init-declarator ([dcl.decl]) or a mem-initializer see https://eel.is/c++draft/intro.execution#5.4

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.
Expand Down
65 changes: 65 additions & 0 deletions clang/test/Modules/pr68702.cpp
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" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the new standard nowadays? Can we use #pragma clang module build and alike to express the module setup?

Copy link
Member Author

Choose a reason for hiding this comment

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

split-file is how many of the modules tests are written, and I find it very handy to reproduce setups with a number of smaller files...

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 -frewrite-imports isn't totally robust yet/needs some more investment) so we had more test coverage for it because it'd be really great if it worked well/was considered when adding new features, etc - and I think it'd be a more direct way to write tests than needing to use the extra split-file invocation, etc (like you could copy a single file/command line to run a modules test, instead of splitting things, having a directory of files to deal with/copy/whatever, etc))

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);
}