[PATCH] D138319: [analyzer] Prepare structures for integral cast feature introducing

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I spent some time on this patch. But I'm left thinking about why we need the 
extra indirection?
Couldn't we just use a `std::pair` pair as a key instead?
You mention //effective// bitwidths in your summary, but the code does not 
mention this concept.
Maybe you could elaborate on what //effective// bitwidth means in this context 
and how this changes the eqclass lookups.
In addition to this, I'd be curious about an informal proof of the correctness.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138319/new/

https://reviews.llvm.org/D138319

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138319: [analyzer] Prepare structures for integral cast feature introducing

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, MaskRay, martong, steakhal.
ASDenysPetrov added a project: clang.
Herald added subscribers: manas, StephenFan, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
ASDenysPetrov requested review of this revision.
Herald added a subscriber: cfe-commits.

Change structures of storing bindings between Symbols and Equivalent Classes.
Add a Bitwidth characteristic as a common feature of integral Symbol to make 
`(char)(int x)` and `(uchar)(int x)` treated under the same Equivalent Class.

The link **Symbol **- **Class **was direct and now it depends on the 
effective(minimal) Bitwidth of the Symbol.
The link **Class **- **Symbol **stays as previously.

Some test cases are affected by regression for the sake of the next patch which 
will fix it.

This patch does not introduce integral casts but prepares the field for the 
core patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138319

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/svalbuilder-casts.cpp

Index: clang/test/Analysis/svalbuilder-casts.cpp
===
--- clang/test/Analysis/svalbuilder-casts.cpp
+++ clang/test/Analysis/svalbuilder-casts.cpp
@@ -39,17 +39,17 @@
 
   // These are not truncated to short as zero.
   static_assert((short)1 != 0, "");
-  clang_analyzer_eval(x == 1);   // expected-warning{{FALSE}}
+  clang_analyzer_eval(x == 1);   // expected-warning{{UNKNOWN}}
   static_assert((short)-1 != 0, "");
-  clang_analyzer_eval(x == -1);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(x == -1);  // expected-warning{{UNKNOWN}}
   static_assert((short)65537 != 0, "");
-  clang_analyzer_eval(x == 65537);   // expected-warning{{FALSE}}
+  clang_analyzer_eval(x == 65537);   // expected-warning{{UNKNOWN}}
   static_assert((short)-65537 != 0, "");
-  clang_analyzer_eval(x == -65537);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(x == -65537);  // expected-warning{{UNKNOWN}}
   static_assert((short)131073 != 0, "");
-  clang_analyzer_eval(x == 131073);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(x == 131073);  // expected-warning{{UNKNOWN}}
   static_assert((short)-131073 != 0, "");
-  clang_analyzer_eval(x == -131073); // expected-warning{{FALSE}}
+  clang_analyzer_eval(x == -131073); // expected-warning{{UNKNOWN}}
 
   // Check for implicit cast.
   short s = y;
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Basic/JsonSupport.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
@@ -872,17 +873,18 @@
 }
 LLVM_DUMP_METHOD void RangeSet::dump() const { dump(llvm::errs()); }
 
-REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(SymbolSet, SymbolRef)
-
 namespace {
 class EquivalenceClass;
+using BitWidthType = uint32_t;
 } // end anonymous namespace
 
-REGISTER_MAP_WITH_PROGRAMSTATE(ClassMap, SymbolRef, EquivalenceClass)
+REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(SymbolSet, SymbolRef)
+REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(ClassSet, EquivalenceClass)
+REGISTER_MAP_FACTORY_WITH_PROGRAMSTATE(ClassMap, BitWidthType, EquivalenceClass)
+
+REGISTER_MAP_WITH_PROGRAMSTATE(SymClassMap, SymbolRef, ClassMap)
 REGISTER_MAP_WITH_PROGRAMSTATE(ClassMembers, EquivalenceClass, SymbolSet)
 REGISTER_MAP_WITH_PROGRAMSTATE(ConstraintRange, EquivalenceClass, RangeSet)
-
-REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(ClassSet, EquivalenceClass)
 REGISTER_MAP_WITH_PROGRAMSTATE(DisequalityMap, EquivalenceClass, ClassSet)
 
 namespace {
@@ -989,11 +991,10 @@
 return getRepresentativeSymbol()->getType();
   }
 
-  EquivalenceClass() = delete;
+  EquivalenceClass() = default;
   EquivalenceClass(const EquivalenceClass &) = default;
-  EquivalenceClass =(const EquivalenceClass &) = delete;
-  EquivalenceClass(EquivalenceClass &&) = default;
-  EquivalenceClass =(EquivalenceClass &&) = delete;
+  EquivalenceClass(SymbolRef Sym) : ID(reinterpret_cast(Sym)) {}
+  EquivalenceClass =(const EquivalenceClass &) = default;
 
   bool operator==(const EquivalenceClass ) const {
 return ID == Other.ID;
@@ -1010,9 +1011,6 @@
   void Profile(llvm::FoldingSetNodeID ) const { Profile(ID, this->ID); }
 
 private:
-  /* implicit */ EquivalenceClass(SymbolRef Sym)
-  : ID(reinterpret_cast(Sym)) {}
-
   /// This function is intended to be used ONLY within the class.