Fix crash with modules and constexpr destructor #69076

hahnjo · 2023-10-14T19:51:08Z

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 #68702

llvmbot · 2023-10-14T19:52:14Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Jonas Hahnfeld (hahnjo)

Changes

Closes #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 --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 &Value, const ASTContext &Ctx,
     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 000000000000000..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 <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" {
+  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);
+}

vgvassilev · 2023-10-17T03:04:32Z

clang/test/Modules/pr68702.cpp

+}
+
+//--- module.modulemap
+module "M" {


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

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...

(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))

clang/test/Modules/pr68702.cpp

hahnjo · 2023-10-25T11:47:48Z

ping @cor3ntin @shafik, could you have a look here?

ChuanqiXu9

While the change itself looks neat, I am curious about the reason how this interact with modules.

shafik · 2023-10-26T02:38:19Z

clang/lib/AST/ExprConstant.cpp

-        EStatus.HasSideEffects)
-      return false;
+    {
+      FullExpressionRAII Scope(Info);


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

shafik · 2023-10-26T02:40:58Z

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.

shafik · 2023-10-26T02:44:17Z

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.

shafik · 2023-10-26T02:48:23Z

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.

hahnjo · 2023-10-30T09:57:02Z

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...

hahnjo · 2023-11-15T15:53:54Z

ping @shafik @cor3ntin @ChuanqiXu9, how can we make progress here?

ChuanqiXu9 · 2023-11-16T03:06:57Z

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.

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 llvm#68702

hahnjo · 2023-12-27T22:36:57Z

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.

ChuanqiXu9 · 2023-12-28T02:03:38Z

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

hahnjo · 2024-01-08T07:20:27Z

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?

ChuanqiXu9 · 2024-01-08T07:24:51Z

clang/lib/AST/ExprConstant.cpp

+      // 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).


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.

ChuanqiXu9 · 2024-01-08T07:26:56Z

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 1~2 weeks if there is no other comments.

hahnjo · 2024-01-08T07:30:33Z

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...

ChuanqiXu9 · 2024-01-08T07:35:16Z

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.

shafik · 2024-01-09T23:23:03Z

ping @shafik @cor3ntin @ChuanqiXu9, how can we make progress here?

Please add a release note and address my previous comment: #69076 (comment)

CC @cor3ntin

hahnjo · 2024-01-10T07:34:48Z

address my previous comment: #69076 (comment)

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?

cor3ntin · 2024-01-10T07:38:53Z

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!

hahnjo · 2024-01-10T07:49:12Z

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.

ChuanqiXu9

LGTM. Thanks for your patience.

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 llvm#68702

hahnjo requested review from cor3ntin and shafik October 14, 2023 19:51

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Oct 14, 2023

This was referenced Oct 14, 2023

Crash with modules and constexpr destructor #68702

Closed

[clang] Fix crash with constexpr destructor root-project/root#13859

Merged

vgvassilev reviewed Oct 17, 2023

View reviewed changes

clang/test/Modules/pr68702.cpp Outdated Show resolved Hide resolved

ChuanqiXu9 reviewed Oct 25, 2023

View reviewed changes

shafik reviewed Oct 26, 2023

View reviewed changes

hahnjo force-pushed the constexpr-destructor-modules branch from b567f28 to a55ca99 Compare December 27, 2023 22:32

ChuanqiXu9 reviewed Jan 8, 2024

View reviewed changes

hahnjo added 2 commits January 10, 2024 08:41

Expand comment

8381d35

Add a release note

676c7ea

ChuanqiXu9 approved these changes Jan 15, 2024

View reviewed changes

hahnjo merged commit 844f833 into llvm:main Jan 15, 2024

hahnjo deleted the constexpr-destructor-modules branch January 15, 2024 07:40

Fix crash with modules and constexpr destructor #69076

Fix crash with modules and constexpr destructor #69076

Conversation

hahnjo commented Oct 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

hahnjo Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

dwblaikie Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hahnjo commented Oct 25, 2023

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

shafik Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

shafik commented Oct 26, 2023

Uh oh!

shafik commented Oct 26, 2023

Uh oh!

shafik commented Oct 26, 2023

Uh oh!

hahnjo commented Oct 30, 2023

Uh oh!

hahnjo commented Nov 15, 2023

Uh oh!

ChuanqiXu9 commented Nov 16, 2023

Uh oh!

hahnjo commented Dec 27, 2023

Uh oh!

ChuanqiXu9 commented Dec 28, 2023

Uh oh!

hahnjo commented Jan 8, 2024

Uh oh!

ChuanqiXu9 Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

hahnjo Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hahnjo commented Jan 8, 2024

Uh oh!

ChuanqiXu9 commented Jan 8, 2024

Uh oh!

shafik commented Jan 9, 2024

Uh oh!

hahnjo commented Jan 10, 2024

Uh oh!

cor3ntin commented Jan 10, 2024

Uh oh!

hahnjo commented Jan 10, 2024

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hahnjo commented Oct 14, 2023 •

edited

Loading

llvmbot commented Oct 14, 2023 •

edited

Loading

ChuanqiXu9 commented Jan 8, 2024 •

edited

Loading