[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Martin Böhme via Phabricator via cfe-commits]

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