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

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added a comment.

Thanks!  Updated the patch.



================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:650
+      assert(FieldsForInit.size() + R->getNumBases() == Inits.size());
+      for ([[maybe_unused]] const CXXBaseSpecifier &Base : R->bases()) {
+        assert(InitIdx < Inits.size());
----------------
mboehme wrote:
> This `[[maybe_unused]]` seems unnecesary, as `Base` is definitely used in the `isLambda()` check?
I removed the isLambda check below from this patch, let me keep this line for now


================
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.
----------------
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 :(


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:683-686
+      // `ModeledFields` also contains from all the bases, but only the modeled
+      // ones. Having an initializer list for a struct basically populates all
+      // the fields for the struct, so the fields set that is populated here
+      // should match the modeled fields without additional filtering.
----------------
mboehme wrote:
> Rewrote this a bit with the aim of making it clearer -- WDYT?
Much better, applied!


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2191
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
----------------
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.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2208-2220
+        const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+        const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+        const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+
+        const Environment &Env =
+              getEnvironmentAtAnnotation(Results, "p");
----------------
mboehme wrote:
> 
(Now this part is removed)


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284