Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][dataflow] fix assert in Environment::getResultObjectLocation #79608

Conversation

paulsemel
Copy link
Contributor

When calling Environment::getResultObjectLocation with a CXXOperatorCallExpr that is a prvalue, we just hit an assert because no record was ever created.

When calling `Environment::getResultObjectLocation` with a
CXXOperatorCallExpr that is a prvalue, we just hit an assert because no
record was ever created.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Jan 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 26, 2024

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Paul Semel (paulsemel)

Changes

When calling Environment::getResultObjectLocation with a CXXOperatorCallExpr that is a prvalue, we just hit an assert because no record was ever created.


Full diff: https://github.com/llvm/llvm-project/pull/79608.diff

1 Files Affected:

  • (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+5)
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 2271a75fbcaf709..3b028a3200b72b2 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -536,6 +536,11 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 
       copyRecord(*LocSrc, *LocDst, Env);
       Env.setStorageLocation(*S, *LocDst);
+    } else {
+      // CXXOperatorCallExpr can be prvalues, in which case we must create a
+      // record for them in order for `Environment::getResultObjectLocation()`
+      // to be able to return a value.
+      VisitCallExpr(S);
     }
   }
 

Copy link
Contributor

@martinboehme martinboehme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!

Can you add a test that fails without the fix? See the existing test ResultObjectLocation in TransferTest.cpp -- you should be able to create a new test based on this that has a CXXOperatorCallExpr returning a record by value.

clang/lib/Analysis/FlowSensitive/Transfer.cpp Outdated Show resolved Hide resolved
Co-authored-by: martinboehme <mboehme@google.com>
@paulsemel
Copy link
Contributor Author

The added test aborts (assert fails) without this patch.

@paulsemel paulsemel force-pushed the paulsemel/fix-flow-sensitive-cxx-operator-call-expr-record-value branch from 9a6a383 to 7375c69 Compare January 31, 2024 13:31
@paulsemel paulsemel merged commit 5c2da28 into llvm:main Jan 31, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants