Improve user feedback about conflicting constraints (#708)

Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
Co-authored-by: John Kastner <[email protected]>

Author: 3 people

clang/include/clang/3C/3CInteractiveData.h

@@ -16,18 +16,26 @@
 #include "clang/3C/ConstraintVariables.h"
 #include "clang/3C/PersistentSourceLoc.h"
 
-// Source info and reason for each wild pointer.
-class WildPointerInferenceInfo {
+// Source info and reason
+class RootCauseDiagnostic {
 public:
-  WildPointerInferenceInfo(std::string Reason, const PersistentSourceLoc PSL)
-      : WildPtrReason(Reason), Location(PSL) {}
+  RootCauseDiagnostic() = default;
+  explicit RootCauseDiagnostic(ReasonLoc &Rsn) : Main(Rsn) {}
 
-  const std::string &getWildPtrReason() const { return WildPtrReason; }
-  const PersistentSourceLoc &getLocation() const { return Location; }
+  const std::string &getReason() { return Main.Reason; }
+  void setReason(const std::string &Rsn) { Main.Reason = Rsn; }
+
+  const PersistentSourceLoc &getLocation() const { return Main.Location; }
+
+  void addReason(const ReasonLoc &Rsn) {
+    Supplemental.push_back(Rsn);
+  }
+
+  std::vector<ReasonLoc> &additionalNotes() { return Supplemental; }
 
 private:
-  std::string WildPtrReason = "";
-  PersistentSourceLoc Location;
+  ReasonLoc Main;
+  std::vector<ReasonLoc> Supplemental;
 };
 
 // Constraints information.
@@ -44,7 +52,7 @@ class ConstraintsInfo {
   void printRootCauseStats(raw_ostream &O, Constraints &CS);
   int getNumPtrsAffected(ConstraintKey CK);
 
-  std::map<ConstraintKey, WildPointerInferenceInfo> RootWildAtomsWithReason;
+  std::map<ConstraintKey, RootCauseDiagnostic> RootWildAtomsWithReason;
   CVars AllWildAtoms;
   CVars InSrcWildAtoms;
   CVars TotalNonDirectWildAtoms;

clang/include/clang/3C/CheckedRegions.h

@@ -81,15 +81,15 @@ class CheckedRegionFinder
   bool containsUncheckedPtr(clang::QualType Qt);
   bool containsUncheckedPtrAcc(clang::QualType Qt, std::set<std::string> &Seen);
   bool isUncheckedStruct(clang::QualType Qt, std::set<std::string> &Seen);
-  void emitCauseDiagnostic(PersistentSourceLoc *);
+  void emitCauseDiagnostic(PersistentSourceLoc);
   bool isWild(CVarOption CVar);
 
   clang::ASTContext *Context;
   clang::Rewriter &Writer;
   ProgramInfo &Info;
   std::set<llvm::FoldingSetNodeID> &Seen;
   std::map<llvm::FoldingSetNodeID, AnnotationNeeded> &Map;
-  std::set<PersistentSourceLoc *> Emitted;
+  std::set<PersistentSourceLoc> Emitted;
   bool EmitWarnings;
 };
 

clang/include/clang/3C/ConstraintResolver.h

@@ -71,7 +71,8 @@ class ConstraintResolver {
 
   CVarSet getInvalidCastPVCons(CastExpr *E);
 
-  CVarSet addAtomAll(CVarSet CVS, ConstAtom *PtrTyp, Constraints &CS);
+  CVarSet addAtomAll(CVarSet CVS, ConstAtom *PtrTyp,
+                     ReasonLoc &Rsn, Constraints &CS);
   CVarSet pvConstraintFromType(QualType TypE);
 
   CSetBkeyPair getAllSubExprConstraintVars(std::vector<Expr *> &Exprs);

clang/include/clang/3C/ConstraintVariables.h

@@ -142,10 +142,7 @@ class ConstraintVariable {
   virtual bool solutionEqualTo(Constraints &, const ConstraintVariable *,
                                bool ComparePtyp = true) const = 0;
 
-  virtual void constrainToWild(Constraints &CS,
-                               const std::string &Rsn) const = 0;
-  virtual void constrainToWild(Constraints &CS, const std::string &Rsn,
-                               PersistentSourceLoc *PL) const = 0;
+  virtual void constrainToWild(Constraints &CS, const ReasonLoc &Rsn) const = 0;
 
   // Return true if this variable was checked in the input. Checked variables
   // might solve to WILD, and unchecked variables might solve to checked. Use
@@ -177,7 +174,7 @@ class ConstraintVariable {
   virtual bool hasNtArr(const EnvironmentMap &E, int AIdx = -1) const = 0;
 
   // Force use of equality constraints in function calls for this CV
-  virtual void equateArgumentConstraints(ProgramInfo &I) = 0;
+  virtual void equateArgumentConstraints(ProgramInfo &I, ReasonLoc &Rsn) = 0;
 
   // Internally combine the constraints and other data from the first parameter
   // with this constraint variable. Used with redeclarations, especially of
@@ -208,17 +205,25 @@ class ConstraintVariable {
   // achieve that, we additionally constrain the internal variables to not
   // change.
   //
-  // Some cases in which the itype must not change at all are indicated by
-  // passing a reason for the "root cause of wildness" as ReasonUnchangeable.
-  // Otherwise ReasonUnchangeable should be set to the empty string.
+  // Some cases in which the itype needs to be constrained not change at all are
+  // indicated by passing a non-default reason for the "root cause of wildness"
+  // in ReasonUnchangeable. If the reason is DEFAULT_REASON, this is a sentinel
+  // meaning that the caller is not requesting such a constraint. Other cases
+  // that need the constraint are detected within equateWithItype itself, and
+  // the appropriate reason is attached there.
+  //
+  // TODO: It looks like there may be some unusual cases in which
+  // equateWithType generates constraints using the reason from
+  // ReasonUnchangeable even if it is the DEFAULT_REASON sentinel. Rethink the
+  // equateWithItype design to figure out what reason should actually be used or
+  // if those constraints should be generated at all.
   virtual void equateWithItype(ProgramInfo &CS,
-                               const std::string &ReasonUnchangeable,
-                               PersistentSourceLoc *PSL) = 0;
+                               const ReasonLoc &ReasonUnchangeable) = 0;
 
   // Copy this variable and replace all VarAtoms with fresh VarAtoms. Using
   // fresh atoms allows the new variable to solve to different types than the
   // original.
-  virtual ConstraintVariable *getCopy(Constraints &CS) = 0;
+  virtual ConstraintVariable *getCopy(ReasonLoc &Rsn, Constraints &CS) = 0;
 
   virtual ~ConstraintVariable(){};
 };
@@ -230,15 +235,15 @@ enum ConsAction { Safe_to_Wild, Wild_to_Safe, Same_to_Same };
 
 void constrainConsVarGeq(const std::set<ConstraintVariable *> &LHS,
                          const std::set<ConstraintVariable *> &RHS,
-                         Constraints &CS, PersistentSourceLoc *PL,
+                         Constraints &CS, const ReasonLoc &Rsn,
                          ConsAction CA, bool DoEqType, ProgramInfo *Info,
                          bool HandleBoundsKey = true);
 void constrainConsVarGeq(ConstraintVariable *LHS, const CVarSet &RHS,
-                         Constraints &CS, PersistentSourceLoc *PL,
+                         Constraints &CS, const ReasonLoc &Rsn,
                          ConsAction CA, bool DoEqType, ProgramInfo *Info,
                          bool HandleBoundsKey = true);
 void constrainConsVarGeq(ConstraintVariable *LHS, ConstraintVariable *RHS,
-                         Constraints &CS, PersistentSourceLoc *PL,
+                         Constraints &CS, const ReasonLoc &Rsn,
                          ConsAction CA, bool DoEqType, ProgramInfo *Info,
                          bool HandleBoundsKey = true);
 
@@ -276,8 +281,7 @@ class PointerVariableConstraint : public ConstraintVariable {
   //       This causes problems if the variable is later used as a deeper
   //       pointer type. See correctcomputation/checkedc-clang#673.
   static PointerVariableConstraint *
-  getWildPVConstraint(Constraints &CS, const std::string &Rsn,
-                      PersistentSourceLoc *PSL = nullptr);
+  getWildPVConstraint(Constraints &CS, const ReasonLoc &Rsn);
 
   // Get constraint variables representing values that are not pointers. If a
   // meaningful name can be assigned to the value, the second method should be
@@ -292,7 +296,7 @@ class PointerVariableConstraint : public ConstraintVariable {
   // by the original constraint variable.
   static PointerVariableConstraint *
   addAtomPVConstraint(PointerVariableConstraint *PVC, ConstAtom *PtrTyp,
-                      Constraints &CS);
+                      ReasonLoc &Rsn, Constraints &CS);
 
   // Return a new constraint variable representing the result of dereferencing
   // the input constraint variable. This is accomplished by first copying the
@@ -518,12 +522,11 @@ class PointerVariableConstraint : public ConstraintVariable {
   void dump() const override { print(llvm::errs()); }
   void dumpJson(llvm::raw_ostream &O) const override;
 
-  void constrainToWild(Constraints &CS, const std::string &Rsn) const override;
-  void constrainToWild(Constraints &CS, const std::string &Rsn,
-                       PersistentSourceLoc *PL) const override;
-  void constrainOuterTo(Constraints &CS, ConstAtom *C, bool DoLB = false,
-                        bool Soft = false);
+  void constrainToWild(Constraints &CS, const ReasonLoc &Rsn) const override;
+  void constrainOuterTo(Constraints &CS, ConstAtom *C, const ReasonLoc &Rsn,
+                        bool DoLB = false, bool Soft = false);
   void constrainIdxTo(Constraints &CS, ConstAtom *C, unsigned int Idx,
+                      const ReasonLoc &Rsn,
                       bool DoLB = false, bool Soft = false);
   bool anyChanges(const EnvironmentMap &E) const override;
   bool anyArgumentIsWild(const EnvironmentMap &E);
@@ -535,15 +538,16 @@ class PointerVariableConstraint : public ConstraintVariable {
   bool isConstantArr() const;
   unsigned long getConstantArrSize() const;
 
-  void equateArgumentConstraints(ProgramInfo &I) override;
+  void equateArgumentConstraints(ProgramInfo &I, ReasonLoc &Rsn) override;
 
   bool isPartOfFunctionPrototype() const { return PartOfFuncPrototype; }
   // Add the provided constraint variable as an argument constraint.
-  bool addArgumentConstraint(ConstraintVariable *DstCons, ProgramInfo &Info);
+  bool addArgumentConstraint(ConstraintVariable *DstCons,
+                             ReasonLoc &Rsn, ProgramInfo &Info);
   // Get the set of constraint variables corresponding to the arguments.
   const std::set<ConstraintVariable *> &getArgumentConstraints() const;
 
-  PointerVariableConstraint *getCopy(Constraints &CS) override;
+  PointerVariableConstraint *getCopy(ReasonLoc &Rsn, Constraints &CS) override;
 
   // Retrieve the atom at the specified index. This function includes special
   // handling for generic constraint variables to create deeper pointers as
@@ -552,8 +556,8 @@ class PointerVariableConstraint : public ConstraintVariable {
 
   ~PointerVariableConstraint() override{};
 
-  void equateWithItype(ProgramInfo &CS, const std::string &ReasonUnchangeable,
-                       PersistentSourceLoc *PSL) override;
+  void equateWithItype(ProgramInfo &CS,
+                       const ReasonLoc &ReasonUnchangeable) override;
 };
 
 typedef PointerVariableConstraint PVConstraint;
@@ -582,7 +586,7 @@ class FVComponentVariable {
       : InternalConstraint(nullptr), ExternalConstraint(nullptr),
         SourceDeclaration("") {}
 
-  FVComponentVariable(FVComponentVariable *Ot, Constraints &CS);
+  FVComponentVariable(FVComponentVariable *Ot, ReasonLoc &Rsn, Constraints &CS);
 
   FVComponentVariable(const clang::QualType &QT, const clang::QualType &ITypeT,
                       clang::DeclaratorDecl *D, std::string N, ProgramInfo &I,
@@ -609,8 +613,8 @@ class FVComponentVariable {
     InternalConstraint->setGenericIndex(idx);
   }
 
-  void equateWithItype(ProgramInfo &CS, const std::string &ReasonUnchangeable,
-                       PersistentSourceLoc *PSL) const;
+  void equateWithItype(ProgramInfo &CS,
+                       const ReasonLoc &ReasonUnchangeable) const;
 
   bool solutionEqualTo(Constraints &CS, const FVComponentVariable *CV,
                        bool ComparePtyp) const;
@@ -640,7 +644,8 @@ class FunctionVariableConstraint : public ConstraintVariable {
   // Count of type parameters (originally from `_Itype_for_any(...)`).
   int TypeParams;
 
-  void equateFVConstraintVars(ConstraintVariable *CV, ProgramInfo &Info) const;
+  void equateFVConstraintVars(ConstraintVariable *CV, ProgramInfo &Info,
+                              ReasonLoc &Rsn) const;
 
 public:
   FunctionVariableConstraint(clang::DeclaratorDecl *D, ProgramInfo &I,
@@ -723,24 +728,22 @@ class FunctionVariableConstraint : public ConstraintVariable {
   void dump() const override { print(llvm::errs()); }
   void dumpJson(llvm::raw_ostream &O) const override;
 
-  void constrainToWild(Constraints &CS, const std::string &Rsn) const override;
-  void constrainToWild(Constraints &CS, const std::string &Rsn,
-                       PersistentSourceLoc *PL) const override;
+  void constrainToWild(Constraints &CS, const ReasonLoc &Rsn) const override;
   bool anyChanges(const EnvironmentMap &E) const override;
   bool hasWild(const EnvironmentMap &E, int AIdx = -1) const override;
   bool hasArr(const EnvironmentMap &E, int AIdx = -1) const override;
   bool hasNtArr(const EnvironmentMap &E, int AIdx = -1) const override;
 
-  void equateArgumentConstraints(ProgramInfo &P) override;
+  void equateArgumentConstraints(ProgramInfo &P, ReasonLoc &Rsn) override;
 
-  FunctionVariableConstraint *getCopy(Constraints &CS) override;
+  FunctionVariableConstraint *getCopy(ReasonLoc &Rsn, Constraints &CS) override;
 
   bool isOriginallyChecked() const override;
   bool isSolutionChecked(const EnvironmentMap &E) const override;
   bool isSolutionFullyChecked(const EnvironmentMap &E) const override;
 
-  void equateWithItype(ProgramInfo &CS, const std::string &ReasonUnchangeable,
-                       PersistentSourceLoc *PSL) override;
+  void equateWithItype(ProgramInfo &CS,
+                       const ReasonLoc &ReasonUnchangeable) override;
 
   ~FunctionVariableConstraint() override {}
 };