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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
