mboehme accepted this revision. mboehme added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2498 +TEST(TransferTest, ReturnStructWithInheritance) { + // This is a crash repro. (Returning a struct involves copy/move constructor) + std::string Code = R"( ---------------- kinu wrote: > mboehme wrote: > > kinu wrote: > > > mboehme wrote: > > > > I don't think this is true -- the `return S1` in `target()` will use > > > > NRVO. > > > > > > > > But I also don't think it's relevant -- I believe the crash this is > > > > reproducing was triggered by the `InitListExpr`, not the return > > > > statement? > > > > > > > > Given the other tests added in this patch, do we need this test? > > > It will use NRVO but in AST this still seems to generate CXXConstructExpr > > > with isCopyOrMoveConstructor() == true because omitting the ctor is not > > > mandatory in compilers. > > > > > > I can drop this one, but I'm also a bit torn because this was the > > > original crash repro that puzzled me a bit. > > > > > > I refined the comment to explain it a bit better; how does it look now? > > > It will use NRVO but in AST this still seems to generate CXXConstructExpr > > > with isCopyOrMoveConstructor() == true > > > > Ah, true, I see this now: > > > > https://godbolt.org/z/z9enG8cW7 > > > > > because omitting the ctor is not mandatory in compilers. > > > > TIL that NRVO [isn't > > guaranteed](https://en.cppreference.com/w/cpp/language/copy_elision). (I > > always thought it was!) > > > > I'm still pretty sure though that the `CXXConstructExpr` will have > > `isElidable() == true`, and in this case we [don't call > > `copyRecord()`](https://github.com/llvm/llvm-project/blob/6c6a2d3445671ada6a58b9ab5ce4a1e11e3dd610/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L474): > > > > ``` > > if (S->isElidable()) { > > if (Value *Val = Env.getValue(*ArgLoc)) > > Env.setValue(*S, *Val); > > } else { > > auto &Val = *cast<RecordValue>(Env.createValue(S->getType())); > > Env.setValue(*S, Val); > > copyRecord(*ArgLoc, Val.getLoc(), Env); > > } > > ``` > > > > So I'm not sure that this repro actually triggers the crash? (Can you > > verify? If it does trigger the crash, where am I going wrong in my thinking > > above?) > > > > > I can drop this one, but I'm also a bit torn because this was the > > > original crash repro that puzzled me a bit. > > > > OK, good to know that this is the original scenario that triggered the > > crash. > > > > I still think it would be OK to keep only > > AssignmentOperatorWithInitAndInheritance because it also triggers a call to > > `copyRecord()` but does so in a more obvious fashion. And I think for a > > test it's actually useful if it's obvious what is happening. > > > I'm still pretty sure though that the `CXXConstructExpr` will have > > `isElidable() == true`, and in this case we [don't call > > `copyRecord()`](https://github.com/llvm/llvm-project/blob/6c6a2d3445671ada6a58b9ab5ce4a1e11e3dd610/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L474): > > Interesting, so I looked into it: > > Looks like after C++17 isElidable is no longer used in AST: > https://github.com/llvm/llvm-project/blob/2a603deec49cb6a27f3a29480ed8a133eef31cee/clang/include/clang/ASTMatchers/ASTMatchers.h#L8351 > > (So this test code doesn't take the branch) > > NRVO seems to be handled around ReturnStmt, not in each Expr: > https://github.com/llvm/llvm-project/blob/2a603deec49cb6a27f3a29480ed8a133eef31cee/clang/lib/AST/Stmt.cpp#L1202 > > > [...] because it also triggers a call to `copyRecord()` but does so in a > > more obvious fashion. And I think for a test it's actually useful if it's > > obvious what is happening. > > Makes sense, now I dropped this. > > > I'm still pretty sure though that the `CXXConstructExpr` will have > > `isElidable() == true`, and in this case we [don't call > > `copyRecord()`](https://github.com/llvm/llvm-project/blob/6c6a2d3445671ada6a58b9ab5ce4a1e11e3dd610/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L474): > > Interesting, so I looked into it: > > Looks like after C++17 isElidable is no longer used in AST: > https://github.com/llvm/llvm-project/blob/2a603deec49cb6a27f3a29480ed8a133eef31cee/clang/include/clang/ASTMatchers/ASTMatchers.h#L8351 > > (So this test code doesn't take the branch) Interesting -- wasn't aware! > NRVO seems to be handled around ReturnStmt, not in each Expr: > https://github.com/llvm/llvm-project/blob/2a603deec49cb6a27f3a29480ed8a133eef31cee/clang/lib/AST/Stmt.cpp#L1202 Thanks for the pointer, makes sese! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159284/new/ https://reviews.llvm.org/D159284 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits