[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/hahnjo closed https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/ChuanqiXu9 approved this pull request. LGTM. Thanks for your patience. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: > Please add a release note > This change needs a release note. Please add an entry to > `clang/docs/ReleaseNotes.rst` in the section the most adapted to the change, > and referencing any Github issue this change fixes. Thanks! Done. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -15754,10 +15754,18 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
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).
hahnjo wrote:
Done.
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/69076
>From a55ca99a373b17501d56d18af9e8aa2dc2cbcea0 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH 1/3] Fix crash with modules and constexpr destructor
With modules, serialization might omit the outer ExprWithCleanups
as it calls ParmVarDecl::getDefaultArg(). Complementary to fixing
this in a separate change, make the code more robust by adding a
FullExpressionRAII and avoid the llvm_unreachable in the added test
clang/test/Modules/pr68702.cpp.
Closes https://github.com/llvm/llvm-project/issues/68702
---
clang/lib/AST/ExprConstant.cpp | 16 ++---
clang/test/Modules/pr68702.cpp | 65 ++
2 files changed, 77 insertions(+), 4 deletions(-)
create mode 100644 clang/test/Modules/pr68702.cpp
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f6aeee1a4e935d..416d48ae82933f 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15754,10 +15754,18 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
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).
+ FullExpressionRAII Scope(Info);
+ if (!EvaluateInPlace(Value, Info, LVal, this,
+ /*AllowNonLiteralTypes=*/true) ||
+ EStatus.HasSideEffects)
+return false;
+}
// At this point, any lifetime-extended temporaries are completely
// initialized.
diff --git a/clang/test/Modules/pr68702.cpp b/clang/test/Modules/pr68702.cpp
new file mode 100644
index 00..d32f946910f4fb
--- /dev/null
+++ b/clang/test/Modules/pr68702.cpp
@@ -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
+ 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 v(100);
+}
>From 8381d35fc3e1dd57ba0dd2a76aea2931c659e419 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Wed, 10 Jan 2024 08:41:59 +0100
Subject: [PATCH 2/3] Expand comment
---
clang/lib/AST/ExprConstant.cpp | 4
1 file changed, 4 insertions(+)
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 416d48ae82933f..f20850d14c0c86 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15760,6 +15760,10 @@ bool Expr::EvaluateAsInitializer(APValue , const
ASTContext ,
// 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);
if (!EvaluateInPlace(Value, Info, LVal, this,
/*AllowNonLiteralTypes=*/true) ||
>From 676c7ea4ad35ae9114023573d5698a22adeb1460 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Wed, 10 Jan 2024 08:47:50 +0100
Subject: [PATCH 3/3] Add a release note
---
clang/docs/ReleaseNotes.rst | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ee211c16a48ac8..aa3252b1d4f5f4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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 `_
+- Fixed a crash with modules and a ``constexpr`` destructor.
+ `Issue 68702 `_
OpenACC Specific Changes
___
[clang] Fix crash with modules and constexpr destructor (PR #69076)
cor3ntin wrote: This change needs a release note. Please add an entry to `clang/docs/ReleaseNotes.rst` in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks! https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: > address my previous comment: [#69076 > (comment)](https://github.com/llvm/llvm-project/pull/69076#issuecomment-1780327252) I had already expanded the commit message with the full details, now also copied to the PR summary. Is that sufficient to address the comment? https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/hahnjo edited https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
shafik wrote: > ping @shafik @cor3ntin @ChuanqiXu9, how can we make progress here? Please add a release note and address my previous comment: https://github.com/llvm/llvm-project/pull/69076#issuecomment-1780327252 CC @cor3ntin https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
ChuanqiXu9 wrote: > Well, this patch is up since almost three months now (!). Sure, we can keep > carrying a similar fix downstream, but ideally I would really like to get rid > of as many local changes as possible. That's not possible without proper > review, but the current situation is quite unsatisfactory... Yeah, fully understood. I have a lot of similar experiences... 1~2 weeks is not long in comparing with 3 months. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: Well, this patch is up since almost three months now (!). Sure, we can keep carrying a similar fix downstream, but ideally I would really like to get rid of as many local changes as possible. That's not possible without proper review, but the current situation is quite unsatisfactory... https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
ChuanqiXu9 wrote: > Ping, is this ok to be accepted and landed? If it is not hurry, I prefer to wait @cor3ntin to have a look. But given the scale of the patch, it should be good too to land it in 2 weeks if there is no other comments. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -15754,10 +15754,18 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
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).
ChuanqiXu9 wrote:
```suggestion
// 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.
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: Ping, is this ok to be accepted and landed? > So personally I am fine with the current workaround with a `FIXME`. You mean next to the comment I already added referring to the C++ standard? Can you formulate what I should put there? https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
ChuanqiXu9 wrote: > I finally had time to debug this: The reason for modules being involved here > is because the serialization code calls `ParmVarDecl::getDefaultArg()` which > strips the outermost `FullExpr`, such as `ExprWithCleanups`. A potential fix > is in #76473 though I'm not really convinced by this asymmetry between > `getInit()` but calling `setDefaultArg()`. However, removing the handling of > `FullExpr` in `setDefaultArg()` causes a total 29 test failures, so that's > not an (easy) option... > > Personally, I would argue that adding `FullExpressionRAII` makes the code > more robust against the absence of `ExprWithCleanups` so that's maybe a good > thing to have regardless of fixing the serialization code. Thanks for looking into this. Good enough for me. Yeah the asymmetry between `getInit()` but calling `setDefaultArg()` is a concerning. Also it may be not easy to understand and fix the underlying problem (why getDefaultArg() will strip the outmost `FullExpr`) **properly**. So personally I am fine with the current workaround with a `FIXME`. Let's wait for the opinion from @cor3ntin and @shafik https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: I finally had time to debug this: The reason for modules being involved here is because the serialization code calls `ParmVarDecl::getDefaultArg()` which strips the outermost `FullExpr`, such as `ExprWithCleanups`. A potential fix is in https://github.com/llvm/llvm-project/pull/76473 though I'm not really convinced by this asymmetry between `getInit()` but calling `setDefaultArg()`. However, removing the handling of `FullExpr` in `setDefaultArg()` causes a total 29 test failures, so that's not an (easy) option... Personally, I would argue that adding `FullExpressionRAII` makes the code more robust against the absence of `ExprWithCleanups` so that's maybe a good thing to have regardless of fixing the serialization code. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/69076
>From a55ca99a373b17501d56d18af9e8aa2dc2cbcea0 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH] Fix crash with modules and constexpr destructor
With modules, serialization might omit the outer ExprWithCleanups
as it calls ParmVarDecl::getDefaultArg(). Complementary to fixing
this in a separate change, make the code more robust by adding a
FullExpressionRAII and avoid the llvm_unreachable in the added test
clang/test/Modules/pr68702.cpp.
Closes https://github.com/llvm/llvm-project/issues/68702
---
clang/lib/AST/ExprConstant.cpp | 16 ++---
clang/test/Modules/pr68702.cpp | 65 ++
2 files changed, 77 insertions(+), 4 deletions(-)
create mode 100644 clang/test/Modules/pr68702.cpp
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f6aeee1a4e935d..416d48ae82933f 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15754,10 +15754,18 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
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).
+ FullExpressionRAII Scope(Info);
+ if (!EvaluateInPlace(Value, Info, LVal, this,
+ /*AllowNonLiteralTypes=*/true) ||
+ EStatus.HasSideEffects)
+return false;
+}
// At this point, any lifetime-extended temporaries are completely
// initialized.
diff --git a/clang/test/Modules/pr68702.cpp b/clang/test/Modules/pr68702.cpp
new file mode 100644
index 00..d32f946910f4fb
--- /dev/null
+++ b/clang/test/Modules/pr68702.cpp
@@ -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
+ 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 v(100);
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
ChuanqiXu9 wrote: The key point now is the rationale why this change relates to modules. It looks not good to proceed without understanding this. Otherwise we're playing with a black box... And the concrete method, ..., well, I guess we had no choice but debugging it hardly. For example, comparing the behavior of clang with and without the change. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
dwblaikie wrote:
(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))
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: ping @shafik @cor3ntin @ChuanqiXu9, how can we make progress here? https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: I can add the comment as requested, but for the other questions related to full expressions and modules I'd really need input from experts... https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
shafik wrote: > While the change itself looks neat, I am curious about the reason how this > interact with modules. IIUC modules is incidental to the problem, it is just a way we run into an underlying issue that we are not dealing with full-expressions properly in this case. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
shafik wrote: I think this change makes sense but I am not crazy about how we deal w/ full-expressions right now with these `FullExpressionRAII`, it feels very ad-hoc and it takes a bunch of time to understand why they are needed where they are. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
shafik wrote: Please make sure you write a more complete description of the problem and why this solves the problem. The description is usually what goes into the git log and that is very useful for quickly understanding commits and tracking down problems. I know some folks edit the description on commit but honestly it is very helpful for the reviewers of your PR to have a full description up front. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -15604,10 +15604,13 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
- return false;
+{
+ FullExpressionRAII Scope(Info);
shafik wrote:
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
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/ChuanqiXu9 commented: While the change itself looks neat, I am curious about the reason how this interact with modules. https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
hahnjo wrote: ping @cor3ntin @shafik, could you have a look here? https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -0,0 +1,65 @@ +// RUN: rm -rf %t +// RUN: mkdir %t +// RUN: split-file %s %t + +// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps -fmodules-cache-path=%t %t/main.cpp -o %t/main.o hahnjo wrote: No, we don't need it, you are right - the crash happens during parsing / sema https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/69076
>From d149de4d4e00b63e506441b516f35aeb41786408 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH 1/2] Fix crash with modules and constexpr destructor
Closes https://github.com/llvm/llvm-project/issues/68702
---
clang/lib/AST/ExprConstant.cpp | 11 +++---
clang/test/Modules/pr68702.cpp | 65 ++
2 files changed, 72 insertions(+), 4 deletions(-)
create mode 100644 clang/test/Modules/pr68702.cpp
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index e5539dedec02a4b..a97e7bd8140890e 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15604,10 +15604,13 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
- return false;
+{
+ FullExpressionRAII Scope(Info);
+ if (!EvaluateInPlace(Value, Info, LVal, this,
+ /*AllowNonLiteralTypes=*/true) ||
+ EStatus.HasSideEffects)
+return false;
+}
// At this point, any lifetime-extended temporaries are completely
// initialized.
diff --git a/clang/test/Modules/pr68702.cpp b/clang/test/Modules/pr68702.cpp
new file mode 100644
index 000..3f91a1001d1eecc
--- /dev/null
+++ b/clang/test/Modules/pr68702.cpp
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
+ 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 v(100);
+}
>From b567f2878b2892c5273596f2b307cf2d1095661f Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Tue, 17 Oct 2023 09:01:59 +0200
Subject: [PATCH 2/2] Remove -emit-obj from test
---
clang/test/Modules/pr68702.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/Modules/pr68702.cpp b/clang/test/Modules/pr68702.cpp
index 3f91a1001d1eecc..d32f946910f4fb8 100644
--- a/clang/test/Modules/pr68702.cpp
+++ b/clang/test/Modules/pr68702.cpp
@@ -2,7 +2,7 @@
// RUN: mkdir %t
// RUN: split-file %s %t
-// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t %t/main.cpp -o %t/main.o
+// 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
hahnjo wrote:
`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...
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -0,0 +1,65 @@ +// RUN: rm -rf %t +// RUN: mkdir %t +// RUN: split-file %s %t + +// RUN: %clang_cc1 -std=c++20 -emit-obj -fmodules -fimplicit-module-maps -fmodules-cache-path=%t %t/main.cpp -o %t/main.o vgvassilev wrote: I thought this is a frontend issue -- do we need `-emit-obj` here? https://github.com/llvm/llvm-project/pull/69076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
vgvassilev wrote:
Is that the new standard nowadays? Can we use `#pragma clang module build` and
alike to express the module setup?
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Author: Jonas Hahnfeld (hahnjo)
Changes
Closes https://github.com/llvm/llvm-project/issues/68702
---
Full diff: https://github.com/llvm/llvm-project/pull/69076.diff
2 Files Affected:
- (modified) clang/lib/AST/ExprConstant.cpp (+7-4)
- (added) clang/test/Modules/pr68702.cpp (+65)
``diff
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index e5539dedec02a4b..a97e7bd8140890e 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15604,10 +15604,13 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
- return false;
+{
+ FullExpressionRAII Scope(Info);
+ if (!EvaluateInPlace(Value, Info, LVal, this,
+ /*AllowNonLiteralTypes=*/true) ||
+ EStatus.HasSideEffects)
+return false;
+}
// At this point, any lifetime-extended temporaries are completely
// initialized.
diff --git a/clang/test/Modules/pr68702.cpp b/clang/test/Modules/pr68702.cpp
new file mode 100644
index 000..3f91a1001d1eecc
--- /dev/null
+++ b/clang/test/Modules/pr68702.cpp
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
+ 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 v(100);
+}
``
https://github.com/llvm/llvm-project/pull/69076
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix crash with modules and constexpr destructor (PR #69076)
https://github.com/hahnjo created
https://github.com/llvm/llvm-project/pull/69076
Closes https://github.com/llvm/llvm-project/issues/68702
>From d149de4d4e00b63e506441b516f35aeb41786408 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld
Date: Sat, 14 Oct 2023 20:10:28 +0200
Subject: [PATCH] Fix crash with modules and constexpr destructor
Closes https://github.com/llvm/llvm-project/issues/68702
---
clang/lib/AST/ExprConstant.cpp | 11 +++---
clang/test/Modules/pr68702.cpp | 65 ++
2 files changed, 72 insertions(+), 4 deletions(-)
create mode 100644 clang/test/Modules/pr68702.cpp
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index e5539dedec02a4b..a97e7bd8140890e 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15604,10 +15604,13 @@ bool Expr::EvaluateAsInitializer(APValue ,
const ASTContext ,
LValue LVal;
LVal.set(VD);
-if (!EvaluateInPlace(Value, Info, LVal, this,
- /*AllowNonLiteralTypes=*/true) ||
-EStatus.HasSideEffects)
- return false;
+{
+ FullExpressionRAII Scope(Info);
+ if (!EvaluateInPlace(Value, Info, LVal, this,
+ /*AllowNonLiteralTypes=*/true) ||
+ EStatus.HasSideEffects)
+return false;
+}
// At this point, any lifetime-extended temporaries are completely
// initialized.
diff --git a/clang/test/Modules/pr68702.cpp b/clang/test/Modules/pr68702.cpp
new file mode 100644
index 000..3f91a1001d1eecc
--- /dev/null
+++ b/clang/test/Modules/pr68702.cpp
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj -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
+class V {
+public:
+ V() = default;
+
+ constexpr V(int n, const A& a = A()) {}
+};
+
+#endif
+
+//--- inst1.h
+#include "V.h"
+
+static void inst1() {
+ V v;
+}
+
+//--- inst2.h
+#include "V.h"
+
+static void inst2() {
+ V v(100);
+}
+
+//--- module.modulemap
+module "M" {
+ 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 v(100);
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
