Clang omits call to move constructor with aggregate init from template type #61145

danakj · 2023-03-03T04:06:52Z

This code double frees with Clang, when it should not: https://godbolt.org/z/5sfT8sW4j

#include <iostream>
#include <utility>

struct VecIter;

struct Vec {
    Vec() : i(new int[3]) {
        std::cerr << this << " Vec::Vec()\n";
        i[0] = 1;
        i[1] = 2;
        i[2] = 3;
    }
    Vec(Vec&& o) : i(o.i) {
        std::cerr << this << " Vec::Vec&&\n";
        o.i = nullptr;
    }
    Vec& operator=(Vec&& o) {
        std::cerr << this << " Vec::operator=&&\n";
        delete[] i;
        i = o.i;
        o.i = nullptr;
        return *this;
    }
    ~Vec() {
        std::cerr << this << " Vec::~Vec\n";
        delete[] i;
    }

    int* i;
};

struct S {
    static S make(auto&& vec) {
        Vec v;
        std::cerr << "Building Vec for S " << &v << "\n";
        v.i[0] = vec.i[0];
        v.i[1] = vec.i[1];
        v.i[2] = vec.i[2];
        std::cerr << "Construct S from Vec " << &v << "\n";
        // Clang bug:
        // The Vec::Vec(Vec&&) constructor is omitted here when constructing S
        // and S::vec.
        //
        // * This does not happen if the parameter is `Vec&&` instead of `auto&&`.
        // * It also does not happen if we uncomment the explicit constructor for S
        //   below.
        return S(std::move(v));
    }

    // Defining this constructor fixes the bug in S::make() and causes the Vec
    // move constructor to be called.
    // S(Vec vec) : vec(std::move(vec)) {}

    Vec vec;
};

int main() {
    auto s = S::make(Vec());
    std::cerr << s.vec.i[0] << "\n";
    std::cerr << s.vec.i[1] << "\n";
    std::cerr << s.vec.i[2] << "\n";
}

Output in GCC:

0x7ffe7c534788 Vec::Vec()
0x7ffe7c534758 Vec::Vec()
Building Vec for S 0x7ffe7c534758
Construct S from Vec 0x7ffe7c534758
0x7ffe7c534780 Vec::Vec&&
0x7ffe7c534758 Vec::~Vec
0x7ffe7c534788 Vec::~Vec
1
2
3
0x7ffe7c534780 Vec::~Vec

Output in Clang:

0x7ffe481779b0 Vec::Vec()
0x7ffe48177958 Vec::Vec()
Building Vec for S 0x7ffe48177958
Construct S from Vec 0x7ffe48177958
0x7ffe48177958 Vec::~Vec
0x7ffe481779b0 Vec::~Vec
0
0
493486096
0x7ffe481779b8 Vec::~Vec
free(): double free detected in tcache 2

Can't show the output of MSVC from godbolt but the code works there.

The text was updated successfully, but these errors were encountered:

danakj · 2023-03-03T04:17:10Z

For what it's worth, in case it's helpful or not, I found this not with an auto&& parameter but with a ConceptName auto&& parameter. Though the use of a concept there is not important in causing the bug to occur, please keep it in mind in case it needs to be considered when fixing this.

There is a serious codegen bug with Clang aggregate init when working with concepts. llvm/llvm-project#61145

llvmbot · 2023-03-07T08:45:25Z

@llvm/issue-subscribers-clang-frontend

llvmbot · 2023-03-07T08:46:12Z

@llvm/issue-subscribers-c-20

shafik · 2023-03-07T17:13:06Z

Note if we change the move line to:

  return S{std::move(v)};

Then gcc and clang match behavior: https://godbolt.org/z/4MeGTvv6E

This looks like a clang bug but not sure what is going wrong.

danakj · 2023-03-15T19:28:44Z

Any chance of this being fixed in Clang 16? Without it the guidance will likely have to be to avoid paren aggregate init, and write tooling to enforce it isn't used.

shafik · 2023-03-15T19:55:05Z

I believe the clang-16 window has closed, I am not really sure where to begin here or how far reaching the issue is. Maybe @zygoloid has an idea of where to poke this.

zygoloid · 2023-03-16T19:36:36Z

I've not been keeping up to date with the parenthesized aggregate init work, but it looks broken enough that I think we should disable it in the Clang 16 release. I'm looking at a reduced testcase with just this statement:

S(std::move(v));

For that, we produce this AST in the template:

| | |   |-CXXFunctionalCastExpr <line:47:9, col:23> 'S':'S' functional cast to S <NoOp>
| | |   | `-CXXParenListInitExpr <col:9, col:22> 'S':'S'
| | |   |   `-CXXConstructExpr <col:11, col:22> 'Vec':'Vec' 'void (Vec &&)'
| | |   |     `-CallExpr <col:11, col:22> 'typename std::remove_reference<Vec &>::type':'Vec' xvalue
| | |   |       |-ImplicitCastExpr <col:11, col:16> 'typename std::remove_reference<Vec &>::type &&(*)(Vec &) noexcept' <BuiltinFnToFnPtr>
| | |   |       | `-DeclRefExpr <col:11, col:16> '<builtin fn type>' Function 0x560455bce5d0 'move' 'typename std::remove_reference<Vec &>::type &&(Vec &) noexcept' (FunctionTemplate 0x56045465c310 'move')
| | |   |       `-DeclRefExpr <col:21> 'Vec':'Vec' lvalue Var 0x560455bc9c18 'v' 'Vec':'Vec'

... which is already wrong: we're missing the CXXBindTemporaryExpr to register the destructor for the S object, and as a consequence we're missing the enclosing ExprWithCleanups, so that temporary won't be destroyed. And that's what happens in practice. (We're presumably missing a call to MaybeBindToTemporary somewhere around where we create the CXXParenListInitExpr.)

Then, after template instantiation happens, we get this AST:

| |     |-CXXFunctionalCastExpr <line:47:9, col:23> 'S':'S' functional cast to S <NoOp>
| |     | `-CXXParenListInitExpr <col:9, col:22> 'S':'S'
| |     |   `-CallExpr <col:11, col:22> 'typename std::remove_reference<Vec &>::type':'Vec' xvalue
| |     |     |-ImplicitCastExpr <col:11, col:16> 'typename std::remove_reference<Vec &>::type &&(*)(Vec &) noexcept' <BuiltinFnToFnPtr>
| |     |     | `-DeclRefExpr <col:11, col:16> '<builtin fn type>' Function 0x5640d51f3d50 'move' 'typename std::remove_reference<Vec &>::type &&(Vec &) noexcept' (FunctionTemplate 0x5640d3c7c310 'move')
| |     |     `-DeclRefExpr <col:21> 'Vec':'Vec' lvalue Var 0x5640d52c02e8 'v' 'Vec':'Vec'

... in which the CXXConstructExpr is gone too. And that results in the Vec being copied with memcpy instead of via a constructor call. (TreeTransform::TransformInitializer has presumably not been updated to properly rebuild a CXXParenListInitExpr.)

zygoloid · 2023-03-16T19:40:36Z

The broken ASTs here will also crash the constant evaluator pretty easily.

shafik · 2023-03-16T19:42:59Z

I think that would be https://reviews.llvm.org/D129531 then

AaronBallman · 2023-03-16T19:59:50Z

I've not been keeping up to date with the parenthesized aggregate init work, but it looks broken enough that I think we should disable it in the Clang 16 release.

Ugh, that's not good. :-( @tstellar @tru -- is it too late for 16.0.0? @zygoloid -- if it is too late for 16.0.0, would it be reasonable to revert in 16.0.1 instead (assuming we can't get the issues resolved in time for .0.1 or they're too large/risky for the dot release)?

zygoloid · 2023-03-16T20:16:44Z

I think that would be https://reviews.llvm.org/D129531 then

FYI: that was reverted and relanded in https://reviews.llvm.org/D141546

That's a very big patch to revert right before a release, even if we have time. :(

shafik · 2023-03-16T20:25:55Z

So I reverted the patch locally but realized this code does not work w/ parens anymore, so it fixes the problem in a way but not very nicely since folks who now rely on this feature will be broken.

I don't have the bandwidth right now to dig into this too much more today.

tstellar · 2023-03-16T20:37:32Z

The 16.0.0 is planned for tomorrow, so I don't think we are going to be able to get this done in time especially if there isn't already a fix. Can we document this issue in the release notes?

alanzhao1 · 2023-03-16T21:08:29Z

So I took a naive stab at this per @zygoloid 's comments and I wrapped the CXXParenListInitExpr with a CXXBindTemporaryExpr

diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index ddb2b5cf5cd1..908642e00058 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -9180,6 +9180,15 @@ ExprResult InitializationSequence::Perform(Sema &S,
                                         /*VerifyOnly=*/false, &CurInit);
       if (CurInit.get() && ResultType)
         *ResultType = CurInit.get()->getType();
+      if (shouldBindAsTemporary(Entity)) {
+        llvm::errs() << "should bind as temporary, dumping entity\n";
+        Entity.dump();
+        llvm::errs() << "should bind as temporary, dumping CurInit\n";
+        CurInit.get()->dump();
+        CurInit = S.MaybeBindToTemporary(CurInit.get());
+        llvm::errs() << "Dumping CurInit after bind to temporary\n";
+        CurInit.get()->dump();
+      }
       break;
     }
     }

I tried building the crashing example (https://godbolt.org/z/G88zerqnK) and we are indeed creating the CXXBindTemporaryExpr, but it looks like that's not sufficient to fix the crash:

should bind as temporary, dumping entity
Temporary 'S'
should bind as temporary, dumping CurInit
CXXParenListInitExpr 0x559f26228c00 'S':'struct S'
`-CXXConstructExpr 0x559f26228bd0 'Vec':'struct Vec' 'void (Vec &&)'
  `-CallExpr 0x559f26228ab0 'typename std::remove_reference<struct Vec &>::type':'struct Vec' xvalue
    |-ImplicitCastExpr 0x559f26228a98 'typename std::remove_reference<struct Vec &>::type &&(*)(struct Vec &) noexcept' <BuiltinFnToFnPtr>
    | `-DeclRefExpr 0x559f26228a08 '<builtin fn type>' Function 0x559f26228810 'move' 'typename std::remove_reference<struct Vec &>::type &&(struct Vec &) noexcept' (FunctionTemplate 0x559f24f58670 'move')
    `-DeclRefExpr 0x559f262280d8 'Vec':'struct Vec' lvalue Var 0x559f26227f18 'v' 'Vec':'struct Vec'
Dumping CurInit after bind to temporary
CXXBindTemporaryExpr 0x559f26228c68 'S':'struct S' (CXXTemporary 0x559f26228c68)
`-CXXParenListInitExpr 0x559f26228c00 'S':'struct S'
  `-CXXConstructExpr 0x559f26228bd0 'Vec':'struct Vec' 'void (Vec &&)'
    `-CallExpr 0x559f26228ab0 'typename std::remove_reference<struct Vec &>::type':'struct Vec' xvalue
      |-ImplicitCastExpr 0x559f26228a98 'typename std::remove_reference<struct Vec &>::type &&(*)(struct Vec &) noexcept' <BuiltinFnToFnPtr>
      | `-DeclRefExpr 0x559f26228a08 '<builtin fn type>' Function 0x559f26228810 'move' 'typename std::remove_reference<struct Vec &>::type &&(struct Vec &) noexcept' (FunctionTemplate 0x559f24f58670 'move')
      `-DeclRefExpr 0x559f262280d8 'Vec':'struct Vec' lvalue Var 0x559f26227f18 'v' 'Vec':'struct Vec'
clang++: /usr/local/google/home/ayzhao/src/llvm-project/clang/include/clang/AST/APValue.h:544: clang::APValue &clang::APValue::getStructField(unsigned int): Assertion `isStruct() && "Invalid accessor"' failed.

(followed by the same stacktrace from godbolt.)

zygoloid · 2023-03-16T21:56:37Z

There are two different bugs here; the constant evaluator crash is due to the template instantiation bug. You should be able to see whether the bind-to-temporary fix is effective by compiling S(std::move(v)); outside of a template and checking whether S's destructor gets called at the end of the statement.

AaronBallman · 2023-03-17T12:00:31Z

The 16.0.0 is planned for tomorrow, so I don't think we are going to be able to get this done in time especially if there isn't already a fix. Can we document this issue in the release notes?

I think we should remove an existing release note: https://github.com/llvm/llvm-project/blob/release/16.x/clang/docs/ReleaseNotes.rst?plain=1#L254 Can you remove that one from the release branch? I'll update https://clang.llvm.org/cxx_status.html accordingly as well from Clang 16 back to No.

CC @tstellar @tru

We thought we had implemented these papers appropriately but have since discovered significant issues. See discussion of the issues at: #61145 The work already done on these papers is remaining in tree for the moment while people investigate whether the issues can be fixed forward in main. The status page is being updated so the status is clear to users of the upcoming Clang 16 release.

tru · 2023-03-17T12:22:34Z

I will push 68b3396 to the release branch.

Done!

We thought we had implemented these papers appropriately but have since discovered significant issues. See discussion of the issues at: llvm/llvm-project#61145 The work already done on these papers is remaining in tree for the moment while people investigate whether the issues can be fixed forward in main. The status page is being updated so the status is clear to users of the upcoming Clang 16 release.

danakj · 2023-03-17T12:50:40Z

When there's a potential patch fix, I will be happy to try it against subspace, which is where I found this bug among others.

alanzhao1 · 2023-03-30T22:12:46Z

Now that I pushed 7df3c71 to main, do I have to cherry pick that change to another branch @royjacobson?

royjacobson · 2023-03-30T22:14:52Z

/cherry-pick 7df3c71

llvmbot · 2023-03-30T22:22:29Z

/branch llvm/llvm-project-release-prs/issue61145

llvmbot · 2023-03-30T22:30:39Z

/pull-request llvm/llvm-project-release-prs#407

H-G-Hristov · 2023-04-08T09:02:53Z

Are P0960R3 and P1975R0 considered unsupported still?

Parenthesized initialization of aggregates	P0960R3	No
Parenthesized initialization of aggregates	P0960R3	No

https://clang.llvm.org/cxx_status.html

alanzhao1 · 2023-04-09T21:15:52Z

@H-G-Hristov There is also #61567, but (I think) that isn't a miscomple and unlike this bug, that bug doesn't cause the compiler to crash. @AaronBallman any thoughts?

AaronBallman · 2023-04-10T16:16:30Z

@H-G-Hristov There is also #61567, but (I think) that isn't a miscomple and unlike this bug, that bug doesn't cause the compiler to crash. @AaronBallman any thoughts?

I think these are distinct issues, yes.

alanzhao1 · 2023-04-10T16:22:54Z

Sorry, I was unclear - my question was whether resolving this issue is sufficient to mark P0960R3 as supported in Clang 16.x, or do we still need to resolve #61567 before doing so?

AaronBallman · 2023-04-10T16:37:32Z

Oh, thank you for clarifying! :-) Hmm this is a tough one. #61567 is about false positive warning diagnostics, which is usually a matter of QoI and so not really something that would hold us back from claiming support for P0960R3. However, this bit worries me:

This will affect uses of C++23 std::ranges::elements_of, which is an aggregate with a reference member that is intended to be used exactly like this.

largely because there's a significant number of users who compile with -Werror and I worry those folks will need to disable the warning more aggressively (and then forget to ever enable it again) in order to accept idiomatic code. I think we want to be more conservative than we've been in the past with claiming support for features, so I think I'd err on the side of not claiming full support (but certainly we can change the No into Partial with a <details> tag). WDYT?

timsong-cpp · 2023-04-13T01:59:10Z

Oh, thank you for clarifying! :-) Hmm this is a tough one. #61567 is about false positive warning diagnostics, which is usually a matter of QoI and so not really something that would hold us back from claiming support for P0960R3.

It's also about false positive errors, not just warnings.

alanzhao1 · 2023-04-15T00:02:03Z

Agreed on holding off on claiming full support especially since I have a patch for #61567 (https://reviews.llvm.org/D148274)

danakj · 2023-04-26T16:13:22Z

How would one know if this has made it into a Clang 16 release?

tru · 2023-04-26T16:52:49Z

It's queued for merge to the release branch and will most likely end up in 16.0.3

Peach1 · 2023-04-27T10:05:06Z

Template struct members don't get default initialized when using () parenthesis initialization.
Is a fix needed for this?:

#include <iostream>

template <typename T>
struct Object
{
    T t;
    bool shouldBeTrue = true;
};

int main()
{
    std::cout << Object<int>(123).shouldBeTrue << std::endl; // 0, broken
    std::cout << Object<int>{123}.shouldBeTrue << std::endl; // 1, as expected
}

Example: https://godbolt.org/z/nbdxhcWMT

alanzhao1 · 2023-04-27T21:06:22Z

@Peach1 that issue is tracked in #62266

* Fix an issue where temporaries initialized via parenthesized aggregate initialization don't get destroyed. * Fix an issue where aggregate initialization omits calls to class members' move constructors after a TreeTransform. This occurs because the CXXConstructExpr wrapping the call to the move constructor gets unboxed during a TreeTransform of the wrapping FunctionalCastExpr (as with a InitListExpr), but unlike InitListExpr, we dont reperform the InitializationSequence for the list's expressions to regenerate the CXXConstructExpr. This patch fixes this bug by treating CXXParenListInitExpr identically to InitListExpr in this regard. Fixes #61145 Reviewed By: rsmith Differential Revision: https://reviews.llvm.org/D146465 (cherry picked from commit 7df3c71)

P0960R3 and P1975R0 were marked not implemented because of llvm#61145, This issue has been fixed and backported to LLVM 16, the status page should reflect that. Differential Revision: https://reviews.llvm.org/D150122

P0960R3 and P1975R0 were marked not implemented because of #61145, This issue has been fixed and backported to LLVM 16, the status page should reflect that. Reviewed By: #clang-language-wg, ayzhao, erichkeane Differential Revision: https://reviews.llvm.org/D150122

github-actions bot added the new issue label Mar 3, 2023

EugeneZelenko added llvm:optimizations and removed new issue labels Mar 3, 2023

danakj added a commit to danakj/subspace that referenced this issue Mar 3, 2023

Avoid aggregate init in Clang 17 still

1cb2238

There is a serious codegen bug with Clang aggregate init when working with concepts. llvm/llvm-project#61145

danakj added a commit to chromium/subspace that referenced this issue Mar 3, 2023

Avoid aggregate init in Clang 17 still

961b9a5

There is a serious codegen bug with Clang aggregate init when working with concepts. llvm/llvm-project#61145

royjacobson added clang:frontend Language frontend issues, e.g. anything involving "Sema" miscompilation and removed llvm:optimizations labels Mar 7, 2023

royjacobson added the c++20 label Mar 7, 2023

royjacobson added this to C++ 20 in Clang Mar 13, 2023

EugeneZelenko reopened this Mar 30, 2023

EugeneZelenko added the release:backport label Mar 30, 2023

llvmbot mentioned this issue Mar 30, 2023

PR for llvm/llvm-project#61145 llvm/llvm-project-release-prs#407

Merged

tru modified the milestones: LLVM 16.0.1 Release, LLVM 16.0.2 Release Apr 9, 2023

tru added this to LLVM Release Status Apr 26, 2023

github-project-automation bot moved this to Needs Triage in LLVM Release Status Apr 26, 2023

tru moved this from Needs Triage to Needs Merge in LLVM Release Status Apr 26, 2023

tstellar closed this as completed in llvm/llvm-project-release-prs#407 May 2, 2023

tstellar moved this from Needs Merge to Done in LLVM Release Status May 2, 2023

EugeneZelenko added the release:merged label May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang omits call to move constructor with aggregate init from template type #61145

Clang omits call to move constructor with aggregate init from template type #61145

danakj commented Mar 3, 2023

danakj commented Mar 3, 2023

llvmbot commented Mar 7, 2023

llvmbot commented Mar 7, 2023

shafik commented Mar 7, 2023

danakj commented Mar 15, 2023 •

edited

Loading

shafik commented Mar 15, 2023

zygoloid commented Mar 16, 2023 •

edited

Loading

zygoloid commented Mar 16, 2023

shafik commented Mar 16, 2023

AaronBallman commented Mar 16, 2023

zygoloid commented Mar 16, 2023

shafik commented Mar 16, 2023

tstellar commented Mar 16, 2023

alanzhao1 commented Mar 16, 2023

zygoloid commented Mar 16, 2023

AaronBallman commented Mar 17, 2023 •

edited

Loading

tru commented Mar 17, 2023 •

edited

Loading

danakj commented Mar 17, 2023

alanzhao1 commented Mar 30, 2023

royjacobson commented Mar 30, 2023

llvmbot commented Mar 30, 2023

llvmbot commented Mar 30, 2023

H-G-Hristov commented Apr 8, 2023

alanzhao1 commented Apr 9, 2023

AaronBallman commented Apr 10, 2023

alanzhao1 commented Apr 10, 2023

AaronBallman commented Apr 10, 2023

timsong-cpp commented Apr 13, 2023

alanzhao1 commented Apr 15, 2023

danakj commented Apr 26, 2023

tru commented Apr 26, 2023

Peach1 commented Apr 27, 2023

alanzhao1 commented Apr 27, 2023

Clang omits call to move constructor with aggregate init from template type #61145

Clang omits call to move constructor with aggregate init from template type #61145

Comments

danakj commented Mar 3, 2023

danakj commented Mar 3, 2023

llvmbot commented Mar 7, 2023

llvmbot commented Mar 7, 2023

shafik commented Mar 7, 2023

danakj commented Mar 15, 2023 • edited Loading

shafik commented Mar 15, 2023

zygoloid commented Mar 16, 2023 • edited Loading

zygoloid commented Mar 16, 2023

shafik commented Mar 16, 2023

AaronBallman commented Mar 16, 2023

zygoloid commented Mar 16, 2023

shafik commented Mar 16, 2023

tstellar commented Mar 16, 2023

alanzhao1 commented Mar 16, 2023

zygoloid commented Mar 16, 2023

AaronBallman commented Mar 17, 2023 • edited Loading

tru commented Mar 17, 2023 • edited Loading

danakj commented Mar 17, 2023

alanzhao1 commented Mar 30, 2023

royjacobson commented Mar 30, 2023

llvmbot commented Mar 30, 2023

llvmbot commented Mar 30, 2023

H-G-Hristov commented Apr 8, 2023

alanzhao1 commented Apr 9, 2023

AaronBallman commented Apr 10, 2023

alanzhao1 commented Apr 10, 2023

AaronBallman commented Apr 10, 2023

timsong-cpp commented Apr 13, 2023

alanzhao1 commented Apr 15, 2023

danakj commented Apr 26, 2023

tru commented Apr 26, 2023

Peach1 commented Apr 27, 2023

alanzhao1 commented Apr 27, 2023

danakj commented Mar 15, 2023 •

edited

Loading

zygoloid commented Mar 16, 2023 •

edited

Loading

AaronBallman commented Mar 17, 2023 •

edited

Loading

tru commented Mar 17, 2023 •

edited

Loading