This is an archive of the discontinued LLVM Phabricator instance.

[clang] Do not discard cleanups while processing immediate invocation
ClosedPublic

Authored by Fznamznon on Jun 28 2023, 4:39 AM.

Details

Summary

Since an immediate invocation is a full expression itself - it requires
an additional ExprWithCleanups node, but it can participate to a bigger
full expression which actually requires cleanups to be run after.

Thanks @ilya-biryukov for helping reducing the reproducer and confirming
that the analysis is correct.

Fixes https://github.com/llvm/llvm-project/issues/60709

Diff Detail

Event Timeline

Fznamznon created this revision.Jun 28 2023, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 4:39 AM
Fznamznon requested review of this revision.Jun 28 2023, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 4:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ilya-biryukov added inline comments.Jun 28 2023, 10:59 AM
clang/lib/Sema/SemaExprCXX.cpp
7374 ↗(On Diff #535419)

Because we may potentially pass some cleanups here (which are outside ConstantExpr) and won't discard them later, we end up adding the unrelated blocks to this cleanup expression.
Here's an example. If we start with this code:

cpp
  struct P {
    consteval P() {}
  };

  struct A {
    A(int v) { this->data = new int(v); }
    const int& get() const {
      return *this->data;
    }
    ~A() { delete data; }
  private:
    int *data;
  };

  void foo() {
    A a(1);
  for (; ^{ ptr = &a.get(); }(), P(), false;);
  }

And run clang++ -std=c++20 -fblocks -Xclang=-ast-dump, we'll get:
(notice same Block in two cleanups)

`-ForStmt 0x5592888b1318 <line:17:3, col:46>
  |-<<<NULL>>>
  |-<<<NULL>>>
  |-ExprWithCleanups 0x5592888b12f0 <col:10, col:39> 'bool'
  | |-cleanup Block 0x5592888ad728
  | `-BinaryOperator 0x5592888b12d0 <col:10, col:39> 'bool' ','
  |   |-BinaryOperator 0x5592888b12a0 <col:10, col:36> 'P':'P' ','
  |   | |-CallExpr 0x5592888adb48 <col:10, col:31> 'void'
  |   | | `-BlockExpr 0x5592888adb30 <col:10, col:29> 'void (^)()'
  |   | |   `-BlockDecl 0x5592888ad728 <col:10, col:29> col:10
  |   | |     |-capture Var 0x5592888ad318 'a' 'A':'A'
  |   | |     `-CompoundStmt 0x5592888ad930 <col:11, col:29>
  |   | `-CXXFunctionalCastExpr 0x5592888b1278 <col:34, col:36> 'P':'P' functional cast to P <NoOp>
  |   |   `-ConstantExpr 0x5592888b1108 <col:34, col:36> 'P':'P'
  |   |     |-value: Struct
  |   |     `-ExprWithCleanups 0x5592888b10e8 <col:34, col:36> 'P':'P'
  |   |       |-cleanup Block 0x5592888ad728
  |   |       `-CXXTemporaryObjectExpr 0x5592888b10b8 <col:34, col:36> 'P':'P' 'void ()'
  |   `-CXXBoolLiteralExpr 0x5592888b12c0 <col:39> 'bool' false

I didn't check if this particular error can lead to codegen bug, but even having a misplaced cleanup in the AST is already a red flag.

I think in case of immediate invocations we could simply create ExprWithCleanups and not do anything else:

  • CleanupVarDeclMarking will be handled when containing evaluation context is popped from the stack.
  • ExprCleanupObjects may only contain blocks at this point (as it's C++ and we have assert for this). One can't use blocks inside a constant expression (Clang will produce an error) so in correct code any cleanups we see inside the current context must handled by the containing evaluation context and not this ExprWithCleanups.

Rebase, don't use MaybeCreateExprWithCleanups

Fznamznon added inline comments.Jun 29 2023, 2:09 AM
clang/lib/Sema/SemaExprCXX.cpp
7374 ↗(On Diff #535419)

I see. Thank you. I updated it to only create ExprWithCleanups.

ilya-biryukov accepted this revision.Jun 29 2023, 3:06 AM

LGTM with two NITs. Thanks a lot for fixing this!

clang/lib/Sema/SemaExpr.cpp
18187

NIT: maybe be a bit more specific about the types of cleanups?

// Note that ExprWithCleanups created here must always have empty cleanup objects:
// - compound literals do not create cleanup objects in C++ and immediate invocations are C++-only.
// - blocks are not allowed inside constant expressions and compiler will issue an error if they appear there.
// Hence, in correct code any cleanup objects created inside current evaluation context must be outside
// the immediate invocation.
clang/test/SemaCXX/consteval-cleanup.cpp
1

Maybe also add a test with -fblocks that checks that only one ExprWithCleanups has the block cleanup action attached?

This revision is now accepted and ready to land.Jun 29 2023, 3:06 AM
Fznamznon updated this revision to Diff 535721.Jun 29 2023, 4:36 AM

Update the comment, add blocks test.

ilya-biryukov accepted this revision.Jun 29 2023, 7:59 AM

LGTM once again with one NIT about the comment line placement.
Thanks!

clang/lib/Sema/SemaExpr.cpp
18198–18199

NIT: put this on a separate line.

This revision was landed with ongoing or failed builds.Jun 30 2023, 1:52 AM
This revision was automatically updated to reflect the committed changes.

LGTM, thank you for doing this!

clang/lib/Sema/SemaExpr.cpp
18201

This argument makes sense to me, thanks for documenting why this is correct!