[clang][dataflow] Fix assert for CXXConstructExpr argument number

[paulsemel]

Summary

It is permissible for a copy constructor to have additional parameters as long as they have default arguments defined for them. Regardless, it doesn't change the way we should treat those constructors in the dataflow framework.

[ymandel]

good catch!

clang/lib/Analysis/FlowSensitive/Transfer.cpp

566

nit: please use explicit comparison: assert(S->getNumArgs() != 0).

[paulsemel]

change assert condition

[ymandel]

Do you have permissions to commit or do you need one of us to do that for you?

[paulsemel]

In D144546#4147618, @ymandel wrote:

Do you have permissions to commit or do you need one of us to do that for you?

I requested them again, as I used to have them back then when SVN was still around!

[steakhal]

By looking at the title, I get the impression that this fixes an assertion violation.
I also observed that this commit is part of main but not part of release/16.x, hence the clang-16 would be released without this fix.

I want to raise awareness of backporting crash fixes to llvm releases. IMO that's a good practice.
So my question is, should we backport this patch to the release branch?

If so, could you please check if there are more commits like this for the dataflow library @ymandel?

I used git log release/16.x..main --oneline clang/lib/{Analysis,AST,ASTMatchers,StaticAnalyzer} clang/include/clang/{Analysis,AST,ASTMatchers,StaticAnalyzer} | grep -i 'crash\|fix\|assert' to check for relevant commits.

[ymandel]

In D144546#4167161, @steakhal wrote:

By looking at the title, I get the impression that this fixes an assertion violation.
I also observed that this commit is part of main but not part of release/16.x, hence the clang-16 would be released without this fix.

I want to raise awareness of backporting crash fixes to llvm releases. IMO that's a good practice.
So my question is, should we backport this patch to the release branch?

If so, could you please check if there are more commits like this for the dataflow library @ymandel?

I used git log release/16.x..main --oneline clang/lib/{Analysis,AST,ASTMatchers,StaticAnalyzer} clang/include/clang/{Analysis,AST,ASTMatchers,StaticAnalyzer} | grep -i 'crash\|fix\|assert' to check for relevant commits.

Yes, this could be backported. I'm happy to look out for this in the future, but I don't seem to have release/16.x in my git repo -- please let me know how to pull that in. I'm only responsible for Analysis/FlowSensitive, but even just there not all fixes have those search terms in there commit message.

[steakhal]

In D144546#4171238, @ymandel wrote:

In D144546#4167161, @steakhal wrote:

By looking at the title, I get the impression that this fixes an assertion violation.
I also observed that this commit is part of main but not part of release/16.x, hence the clang-16 would be released without this fix.

I want to raise awareness of backporting crash fixes to llvm releases. IMO that's a good practice.
So my question is, should we backport this patch to the release branch?

If so, could you please check if there are more commits like this for the dataflow library @ymandel?

I used git log release/16.x..main --oneline clang/lib/{Analysis,AST,ASTMatchers,StaticAnalyzer} clang/include/clang/{Analysis,AST,ASTMatchers,StaticAnalyzer} | grep -i 'crash\|fix\|assert' to check for relevant commits.

Yes, this could be backported. I'm happy to look out for this in the future, but I don't seem to have release/16.x in my git repo -- please let me know how to pull that in. I'm only responsible for Analysis/FlowSensitive, but even just there not all fixes have those search terms in there commit message.

The branch I was referring to was: https://github.com/llvm/llvm-project/tree/release/16.x

For examples of how to do a backport, I would refer to the issue tracker for examples.
Look for issues with the "release:backport" label and repeat what is done there.

I think the steps are something like:

1. Create a PR against llvm/llvm-project where you explain why you think the given change is "safe" to backport.
2. Remember to add all the labels you feel are related.
3. Mention any dependencies that the given commit might have that would also need to be backported prior to porting this one.
4. Tag the code owner. I guess for you it's not relevant.
5. Add the PR to the release "Milestone" and "Project".

Finally, reply to the PR by the /cherry-pick <hash> message so that the llvmbot will jump and try to cherry-pick the given commit to the release branch.
Usually, this will succeed without conflicts, but if you have any it tells you what to do next.

By the end of this, you will have a PR for another repo (llvm/llvm-project-release-prs), where the bot will ping the code owner for approval.

If anything goes wrong you can always tag "tstellar" or "tru" for help.
"nikic" also seems to be involved with the release process.

Note that I don't know if this is the "official" way of doing this, but so far it worked out.

Here is an example where the patch was applied cleanly: llvm/llvm-project#61097

[steakhal]

FYI the release schedule is here: https://llvm.org/docs/HowToReleaseLLVM.html#annual-release-schedule

clang/lib/Analysis/FlowSensitive/Transfer.cpp

     assert(ConstructorDecl != nullptr);
 
     if (ConstructorDecl->isCopyOrMoveConstructor()) {
-      assert(S->getNumArgs() == 1);
+      // It is permissible for a copy/move constructor to have additional
+      // parameters as long as they have default arguments defined for them.
+      assert(S->getNumArgs());
 
       const Expr *Arg = S->getArg(0);
       assert(Arg != nullptr);

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

       });
 }
 
+TEST(TransferTest, CopyConstructorWithDefaultArgument) {
+  std::string Code = R"(
+    struct A {
+      int Baz;
+      A() = default;
+      A(const A& a, bool def = true) { Baz = a.Baz; }
+    };
+
+    void target() {
+      A Foo;
+      (void)Foo.Baz;
+      A Bar = Foo;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+        ASSERT_THAT(FooDecl, NotNull());
+
+        const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+        ASSERT_THAT(BarDecl, NotNull());
+
+        const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+        ASSERT_THAT(BazDecl, NotNull());
+
+        const auto *FooLoc = cast<AggregateStorageLocation>(
+            Env.getStorageLocation(*FooDecl, SkipPast::None));
+        const auto *BarLoc = cast<AggregateStorageLocation>(
+            Env.getStorageLocation(*BarDecl, SkipPast::None));
+
+        const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
+        const auto *BarVal = cast<StructValue>(Env.getValue(*BarLoc));
+        EXPECT_EQ(FooVal, BarVal);
+
+        const auto *FooBazVal =
+            cast<IntegerValue>(Env.getValue(FooLoc->getChild(*BazDecl)));
+        const auto *BarBazVal =
+            cast<IntegerValue>(Env.getValue(BarLoc->getChild(*BazDecl)));
+        EXPECT_EQ(FooBazVal, BarBazVal);
+      });
+}
+
 TEST(TransferTest, CopyConstructorWithParens) {
   std::string Code = R"(
     struct A {