[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances
Martin Böhme via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 5 00:45:53 PDT 2023
mboehme added inline comments.
================
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)));
+ }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:653-654
+ auto Init = Inits[InitIdx++];
+ assert(GetCanonicalType(Base.getType()) ==
+ GetCanonicalType(Init->getType()));
+ if (Base.getType()->getAsCXXRecordDecl()->isLambda()) {
----------------
mboehme wrote:
> I believe this can be
>
> ```
> assert(Base.getType().getCanonicalType() ==
> Init->getType().getCanonicalType());
> ```
>
> - The initializer for a base class should always be a prvalue
> - And I believe the type of a prvalue can't have `const` or `volatile` qualifiers (neither of them really makes sense)
Do you actually need the `Unqualified` (see also comment above)? Do you have an example of a specific scenario where we need to drop the qualifiers?
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:655
+ GetCanonicalType(Init->getType()));
+ if (Base.getType()->getAsCXXRecordDecl()->isLambda()) {
+ // FIXME: Figure out what to do for lambdas.
----------------
kinu wrote:
> mboehme wrote:
> > - Can this actually happen? (How do you make a lambda a base class?)
> > - Why does this need to be handled separately? (Isn't a lambda just a normal class, modulo sugar? What is different about lambdas that means we can't handle them like other classes?)
> >
> > Depending on the answers to the above, would also be useful to capture them as comments in the source code.
> >
> > I assume you encountered this scenario somewhere. Can you add a test (with incorrect behavior marked as a FIXME) that exercises this?
> I hit this case in one of real code so added this afterwards...
>
> but actually can I drop this part from this patch but come back in a separate patch once I clarify the behavior? I somehow lost the note about in which file this happened, and I need to confirm a few things before I can add a test :(
Sounds good!
================
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:
> 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!
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1479-1481
+ for (auto [Field, _] : MDLoc.children()) {
+ Fields.push_back(Field);
+ }
----------------
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1528-1530
+ F.x4 = F.x2;
+ F.x6 = 1;
+ F.x8 = 1;
----------------
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?
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2191
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
----------------
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.
================
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:
> > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D159284/new/
https://reviews.llvm.org/D159284
More information about the cfe-commits
mailing list