kinu created this revision.
Herald added a reviewer: NoQ.
Herald added a project: All.
kinu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This fix still leaves inherited fields from base classes unpopulated
(even if they are accessed in the code), and might cause false
positives, but fix crashes that could happen because of source /
destination mismatches.
More details:
When a struct is declared, the Record value of the struct is usually
initialized with `Environment::createValue()` which internally calls
`getObjectFields()` (via filtering in `DACtx::getModeledFields()`)
to collects all fields from the current and base classes.
However, if a struct is initialized with InitListExpr, its fields are
initialized based on what is returned by `getFieldsForInitListExpr()`,
which doesn't collect fields from base classes. Moreover, if the base
classes have their own InitListExpr, those InitListExpr's are also
visited, but the field values and locations that are initialized by
them are not merged when the child class's InitListExpr is visited.
This doesn't usually result in a crash, except for the cases where the
struct is used to construct a new struct of the same type with copy/move
constructor: in such a case the destination struct is
created with a regular `createValue()`, and therefore also have
fields from the base classes, but the source struct does not.
And when `copyRecord()` tries to copy the fields by looping over
the destination fields, it crashes because the source struct may
not have the fields.
This change adds a new `getChildOrError()` method in `StorageLocation`
and uses it in `copyRecord()`, so that only the fields that exist in
both record can be copied.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D159248
Files:
clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
clang/lib/Analysis/FlowSensitive/RecordOps.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -16,10 +16,8 @@
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Basic/LangStandard.h"
-#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Casting.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -2023,6 +2021,28 @@
});
}
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct B { int Foo; };
+ struct S : public B {};
+ void target() {
+ S S1 = { 1 };
+ S S2;
+ S S3;
+ S1 = S2; // Only Dst has InitListExpr.
+ S3 = S1; // Only Src has InitListExpr.
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ // FIXME: Add tests to check if `Foo` exists in S1, S2, and S3 once
+ // initialization with inheritance is fixed.
+ });
+}
+
TEST(TransferTest, AssignmentOperatorFromCallResult) {
std::string Code = R"(
struct A {};
@@ -2253,6 +2273,49 @@
ASTContext &ASTCtx) {});
}
+TEST(TransferTest, CopyConstructorWithInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct B { int Foo; };
+ struct S : public B {};
+ void target() {
+ S S1 = { 1 };
+ S S2(S1);
+ // [p1]
+ S2.Foo = 2;
+ // [p2]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ // FIXME: Add tests to check if `Foo` exists both in S1 and S2 once
+ // initialization with inheritance is fixed.
+ });
+}
+
+TEST(TransferTest, CopyConstructorWithBaseInitAndInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct Foo { int Bar; };
+ struct B { Foo F = { 1 }; };
+ struct S : public B {};
+ void target() {
+ S S1 = {};
+ S S2(S1);
+ // [p]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ // FIXME: Add tests to check if `Bar` exists both in S1 and S2 once
+ // initialization with inheritance is fixed.
+ });
+}
+
TEST(TransferTest, MoveConstructor) {
std::string Code = R"(
namespace std {
@@ -2328,6 +2391,22 @@
});
}
+TEST(TransferTest, ReturnStructWithInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct B { int Foo; };
+ struct S : public B { };
+ S target() {
+ S S1 = { 1 };
+ return S1;
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {});
+}
+
TEST(TransferTest, BindTemporary) {
std::string Code = R"(
struct A {
Index: clang/lib/Analysis/FlowSensitive/RecordOps.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -36,7 +36,10 @@
assert(compatibleTypes);
for (auto [Field, DstFieldLoc] : Dst.children()) {
- StorageLocation *SrcFieldLoc = Src.getChild(*Field);
+ llvm::ErrorOr<StorageLocation *> SrcField = Src.getChildOrError(*Field);
+ if (!SrcField)
+ continue;
+ StorageLocation *SrcFieldLoc = SrcField.get();
assert(Field->getType()->isReferenceType() ||
(SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -18,6 +18,7 @@
#include "clang/AST/Type.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/Support/Debug.h"
+#include "llvm/Support/ErrorOr.h"
#include <cassert>
#define DEBUG_TYPE "dataflow"
@@ -135,6 +136,19 @@
return It->second;
}
+ /// Returns the child storage location for `D` if it exists.
+ ///
+ /// Same as `getChild()`, but returns llvm::Error if the field `D` does not
+ /// exist. Most users should use `getChild()` instead, except for the code
+ /// that performs deep copy of two records of the same class, where a field
+ /// in one record may not exist in another record.
+ llvm::ErrorOr<StorageLocation *> getChildOrError(const ValueDecl &D) const {
+ auto It = Children.find(&D);
+ if (It == Children.end())
+ return std::errc::invalid_argument;
+ return It->second;
+ }
+
/// Changes the child storage location for a field `D` of reference type.
/// All other fields cannot change their storage location and always retain
/// the storage location passed to the `RecordStorageLocation` constructor.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
