[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-07 Thread via cfe-commits 

https://github.com/martinboehme closed 
https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-07 Thread Yitzhak Mandelbaum via cfe-commits 

https://github.com/ymand approved this pull request.


https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-07 Thread Yitzhak Mandelbaum via cfe-commits 


@@ -799,6 +806,16 @@ PointerValue 
&Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);
 }
 
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {

ymand wrote:

I don't know of any convention. I tend not to anchor on header order, but YMMV. 
 Up to you.

https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-07 Thread Yitzhak Mandelbaum via cfe-commits 


@@ -414,8 +414,15 @@ void Environment::initialize() {
   }
 } else if (MethodDecl->isImplicitObjectMemberFunction()) {
   QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
-  setThisPointeeStorageLocation(
-  cast(createObject(ThisPointeeType)));
+  auto &ThisLoc =
+  cast(createStorageLocation(ThisPointeeType));
+  setThisPointeeStorageLocation(ThisLoc);
+  refreshRecordValue(ThisLoc, *this);
+  // Initialize fields of `*this` with values, but only if we're not
+  // analyzing a constructor; after all, it's the constructor's job to do

ymand wrote:

Yes. thanks

https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-07 Thread via cfe-commits 


@@ -414,8 +414,15 @@ void Environment::initialize() {
   }
 } else if (MethodDecl->isImplicitObjectMemberFunction()) {
   QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
-  setThisPointeeStorageLocation(
-  cast(createObject(ThisPointeeType)));
+  auto &ThisLoc =
+  cast(createStorageLocation(ThisPointeeType));
+  setThisPointeeStorageLocation(ThisLoc);
+  refreshRecordValue(ThisLoc, *this);
+  // Initialize fields of `*this` with values, but only if we're not
+  // analyzing a constructor; after all, it's the constructor's job to do

martinboehme wrote:

Do you mean something like this?

```cxx
struct S {
S() {}
int i = 1;
};
```

If so, the answer is yes:

```
  |-CXXConstructorDecl  col:5 S 'void ()' implicit-inline
  | |-CXXCtorInitializer Field 0xc1eeb90 'i' 'int'
  | | `-CXXDefaultInitExpr  'int' has rewritten init
  | |   `-IntegerLiteral  'int' 1
  | `-CompoundStmt 
```

https://godbolt.org/z/oYP66fMjT

https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-07 Thread via cfe-commits 


@@ -799,6 +806,16 @@ PointerValue 
&Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);
 }
 
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {

martinboehme wrote:

I consciously put this here to be consistent with the ordering in the header 
file. (Do we have a firm convention for this?)

https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-06 Thread Gábor Horváth via cfe-commits 

https://github.com/Xazax-hun approved this pull request.


https://github.com/llvm/llvm-project/pull/84164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-06 Thread via cfe-commits 

llvmbot wrote:



@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: None (martinboehme)


Changes

This is the constructor's job, and we want to be able to test that it does this.


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


5 Files Affected:

- (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h 
(+5) 
- (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+19-2) 
- (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp 
(+1-1) 
- (modified) clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp (+4-1) 
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+63) 


``diff
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h 
b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 62e7af7ac219bc..7f4a8f0341c9ba 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -436,6 +436,11 @@ class Environment {
 return createObjectInternal(&D, D.getType(), InitExpr);
   }
 
+  /// Initializes the fields (including synthetic fields) of `Loc` with values,
+  /// unless values of the field type are not supported or we hit one of the
+  /// limits at which we stop producing values.
+  void initializeFieldsWithValues(RecordStorageLocation &Loc);
+
   /// Assigns `Val` as the value of `Loc` in the environment.
   void setValue(const StorageLocation &Loc, Value &Val);
 
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp 
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index fd7b06efcc7861..e359f037ea3a7a 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -414,8 +414,15 @@ void Environment::initialize() {
   }
 } else if (MethodDecl->isImplicitObjectMemberFunction()) {
   QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
-  setThisPointeeStorageLocation(
-  cast(createObject(ThisPointeeType)));
+  auto &ThisLoc =
+  cast(createStorageLocation(ThisPointeeType));
+  setThisPointeeStorageLocation(ThisLoc);
+  refreshRecordValue(ThisLoc, *this);
+  // Initialize fields of `*this` with values, but only if we're not
+  // analyzing a constructor; after all, it's the constructor's job to do
+  // this (and we want to be able to test that).
+  if (!isa(MethodDecl))
+initializeFieldsWithValues(ThisLoc);
 }
   }
 }
@@ -799,6 +806,16 @@ PointerValue 
&Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);
 }
 
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {
+  llvm::DenseSet Visited;
+  int CreatedValuesCount = 0;
+  initializeFieldsWithValues(Loc, Visited, 0, CreatedValuesCount);
+  if (CreatedValuesCount > MaxCompositeValueSize) {
+llvm::errs() << "Attempting to initialize a huge value of type: "
+ << Loc.getType() << '\n';
+  }
+}
+
 void Environment::setValue(const StorageLocation &Loc, Value &Val) {
   assert(!isa(&Val) || &cast(&Val)->getLoc() == 
&Loc);
 
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp 
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 4c88c46142d64d..4bc6b19f2553fb 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -388,7 +388,6 @@ builtinTransferInitializer(const CFGInitializer &Elt,
 }
   }
   assert(Member != nullptr);
-  assert(MemberLoc != nullptr);
 
   // FIXME: Instead of these case distinctions, we would ideally want to be 
able
   // to simply use `Environment::createObject()` here, the same way that we do
@@ -404,6 +403,7 @@ builtinTransferInitializer(const CFGInitializer &Elt,
 
 ParentLoc->setChild(*Member, InitExprLoc);
   } else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
+assert(MemberLoc != nullptr);
 if (Member->getType()->isRecordType()) {
   auto *InitValStruct = cast(InitExprVal);
   // FIXME: Rather than performing a copy here, we should really be
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp 
b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
index 09f5524e152c9f..5c4d42c6ccdcf8 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
@@ -179,7 +179,10 @@ llvm::Error test::checkDataflowWithNoopAnalysis(
   // -fnodelayed-template-parsing is the default everywhere but on Windows.
   // Set it explicitly so that tests behave the same on Windows as on other
   // platforms.
-  "-fsyntax-only", "-fno-delayed-template-parsing",
+  // Set -Wno-unused-value because it's often desirable in te

[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)

2024-03-06 Thread via cfe-commits 

https://github.com/martinboehme created 
https://github.com/llvm/llvm-project/pull/84164

This is the constructor's job, and we want to be able to test that it does this.


>From a93fad7447fd8772171a374d999fff0695c95b73 Mon Sep 17 00:00:00 2001
From: Martin Braenne 
Date: Wed, 6 Mar 2024 13:07:24 +
Subject: [PATCH] [clang][dataflow] When analyzing ctors, don't initialize
 fields of `*this` with values.

This is the constructor's job, and we want to be able to test that it does this.
---
 .../FlowSensitive/DataflowEnvironment.h   |  5 ++
 .../FlowSensitive/DataflowEnvironment.cpp | 21 ++-
 .../TypeErasedDataflowAnalysis.cpp|  2 +-
 .../Analysis/FlowSensitive/TestingSupport.cpp |  5 +-
 .../Analysis/FlowSensitive/TransferTest.cpp   | 63 +++
 5 files changed, 92 insertions(+), 4 deletions(-)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h 
b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 62e7af7ac219bc..7f4a8f0341c9ba 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -436,6 +436,11 @@ class Environment {
 return createObjectInternal(&D, D.getType(), InitExpr);
   }
 
+  /// Initializes the fields (including synthetic fields) of `Loc` with values,
+  /// unless values of the field type are not supported or we hit one of the
+  /// limits at which we stop producing values.
+  void initializeFieldsWithValues(RecordStorageLocation &Loc);
+
   /// Assigns `Val` as the value of `Loc` in the environment.
   void setValue(const StorageLocation &Loc, Value &Val);
 
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp 
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index fd7b06efcc7861..e359f037ea3a7a 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -414,8 +414,15 @@ void Environment::initialize() {
   }
 } else if (MethodDecl->isImplicitObjectMemberFunction()) {
   QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
-  setThisPointeeStorageLocation(
-  cast(createObject(ThisPointeeType)));
+  auto &ThisLoc =
+  cast(createStorageLocation(ThisPointeeType));
+  setThisPointeeStorageLocation(ThisLoc);
+  refreshRecordValue(ThisLoc, *this);
+  // Initialize fields of `*this` with values, but only if we're not
+  // analyzing a constructor; after all, it's the constructor's job to do
+  // this (and we want to be able to test that).
+  if (!isa(MethodDecl))
+initializeFieldsWithValues(ThisLoc);
 }
   }
 }
@@ -799,6 +806,16 @@ PointerValue 
&Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);
 }
 
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {
+  llvm::DenseSet Visited;
+  int CreatedValuesCount = 0;
+  initializeFieldsWithValues(Loc, Visited, 0, CreatedValuesCount);
+  if (CreatedValuesCount > MaxCompositeValueSize) {
+llvm::errs() << "Attempting to initialize a huge value of type: "
+ << Loc.getType() << '\n';
+  }
+}
+
 void Environment::setValue(const StorageLocation &Loc, Value &Val) {
   assert(!isa(&Val) || &cast(&Val)->getLoc() == 
&Loc);
 
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp 
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 4c88c46142d64d..4bc6b19f2553fb 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -388,7 +388,6 @@ builtinTransferInitializer(const CFGInitializer &Elt,
 }
   }
   assert(Member != nullptr);
-  assert(MemberLoc != nullptr);
 
   // FIXME: Instead of these case distinctions, we would ideally want to be 
able
   // to simply use `Environment::createObject()` here, the same way that we do
@@ -404,6 +403,7 @@ builtinTransferInitializer(const CFGInitializer &Elt,
 
 ParentLoc->setChild(*Member, InitExprLoc);
   } else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
+assert(MemberLoc != nullptr);
 if (Member->getType()->isRecordType()) {
   auto *InitValStruct = cast(InitExprVal);
   // FIXME: Rather than performing a copy here, we should really be
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp 
b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
index 09f5524e152c9f..5c4d42c6ccdcf8 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
@@ -179,7 +179,10 @@ llvm::Error test::checkDataflowWithNoopAnalysis(
   // -fnodelayed-template-parsing is the default everywhere but on Windows.
   // Set it explicitly so that tests behave the same on Windows as on other
   // platf