kinu added a comment.
Thanks, addressed the comments, PTAL~
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:688-690
+ for ([[maybe_unused]] auto [Field, Loc] : FieldLocs) {
+ assert(ModeledFields.contains(cast_or_null<FieldDecl>(Field)));
+ }
----------------
mboehme wrote:
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Does it work even if assert() is dropped? ... looks like it does. Ok, applied.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:672-673
+ // The types are same, or
+ GetCanonicalType(Field->getType()) ==
+ GetCanonicalType(Init->getType()) ||
+ // The field's type is T&, and initializer is T
----------------
mboehme wrote:
> mboehme wrote:
> > Again, this is the prvalue case, so I think you should simply be able to
> > use `QualType::getCanonicalType()`.
> Thinking about this again -- the field can, of course, have qualifiers, while
> the `Init` as a prvalue should not, so I think this should read
>
> ```
> Field->getType().getCanonicalType().getUnqualifiedType() ==
> Init->getType().getCanonicalType()
> ```
>
> Sorry for the back and forth!
No problem, thanks for clarifying the details, done.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1528-1530
+ F.x4 = F.x2;
+ F.x6 = 1;
+ F.x8 = 1;
----------------
mboehme wrote:
> mboehme wrote:
> > These lines seem unnecessary. The test only wants to check whether the
> > fields are modeled, not what their values are.
> >
> > If anything, leaving out these operations makes the test stronger: It then
> > tests whether the fields are modeled even if they are only initialized with
> > an empty `{}`.
> >
> > Then, once we leave out these operations, it's probably sufficient to give
> > each class in this test just a single member variable (which is another
> > nice simplification).
> Reopening the comment to discuss this line:
>
> > Then, once we leave out these operations, it's probably sufficient to give
> > each class in this test just a single member variable (which is another
> > nice simplification).
>
> I see that giving each class two member variables makes this test consistent
> with the test above -- but OTOH, we're testing exactly the same thing for
> both member variables, so maybe it's better on the whole to simplify the test
> by putting just one member variable in each class?
That's true for this test... now I made each class have only one member.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2191
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
----------------
mboehme wrote:
> kinu wrote:
> > mboehme wrote:
> > > IIUC, the crash was happening because `copyRecord()` assumes that the
> > > source and destination have the same fields. More specifically, there was
> > > an unspoken invariant in the framework that a `RecordStorageLocation`
> > > should always contain exactly the fields returned by
> > > `DataflowAnalysisContext::getModeledFields()`), but our treatment of
> > > `InitListExpr` was violating this invariant.
> > >
> > > IOW, this wasn't really an issue with the way we treat copy/move
> > > operations but with `InitlistExpr`.
> > >
> > > I understand that we want to have a test the reproduces the exact
> > > circumstances that led to the crash, but maybe it's enough to have one of
> > > these, and have the focus of the testing be on testing the actual
> > > invariant that we want to maintain? IOW, maybe keep this test but delete
> > > the additional tests below?
> > Dropped the additional tests below.
> I still see more tests below? Suggest dropping these.
Ah I see what you mean. I dropped more!
================
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"(
----------------
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.
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
