* Added shutdown() functions to SmtEngine, TheoryEngine, PropEngine,
authorMorgan Deters <mdeters@gmail.com>
Fri, 12 Mar 2010 19:41:04 +0000 (19:41 +0000)
committerMorgan Deters <mdeters@gmail.com>
Fri, 12 Mar 2010 19:41:04 +0000 (19:41 +0000)
  DecisionEngine, and Theory.  These are triggered from the SmtEngine
  dtor before the other engines are deleted.  This is important
  because of potential issues with outstanding TNodes in Theory
  implementations (which fail if the destruction is done in the wrong
  order, in certain cases).

* Favor "FactQueueResetter" instead of clearAssertionQueues() for
  resetting facts queue in Theory implementations.

* Better theory-rewriting code.

* Minor cleanups.

src/prop/minisat/core/Solver.C
src/prop/prop_engine.h
src/prop/sat.h
src/smt/smt_engine.cpp
src/smt/smt_engine.h
src/theory/theory.h
src/theory/theory_engine.cpp
src/theory/theory_engine.h
src/theory/uf/theory_uf.cpp
src/util/decision_engine.cpp
src/util/decision_engine.h

index 0f8622cb8895691848d07b19d6c872532b024336..a735cd46ce8ee37b319e79a3662f4e7b65c5d59a 100644 (file)
@@ -182,8 +182,6 @@ void Solver::cancelUntil(int level) {
         trail.shrink(trail.size() - trail_lim[level]);
         trail_lim.shrink(trail_lim.size() - level);
     }
-    // Now, clear the TheoryEngine queue
-    proxy->clearAssertionQueues();
 }
 
 
index 2ddbd24fce3927c00f555d78bd4b789700e82980..d699153b295bcd86f293cd4303b8de7d7667f268 100644 (file)
@@ -76,6 +76,16 @@ public:
    */
   CVC4_PUBLIC ~PropEngine();
 
+  /**
+   * This is called by SmtEngine, at shutdown time, just before
+   * destruction.  It is important because there are destruction
+   * ordering issues between some parts of the system (notably between
+   * PropEngine and Theory).  For now, there's nothing to do here in
+   * the PropEngine.
+   */
+  void shutdown() {
+  }
+
   /**
    * Converts the given formula to CNF and assert the CNF to the sat solver.
    * The formula is asserted permanently for the current context.
index ec38bb0193d1e4cb67be1ea5abd50643b65febf2..f29436b97faa599831204f040886b984fd2f8dc1 100644 (file)
@@ -77,8 +77,10 @@ public:
     inline size_t operator()(const SatLiteral& literal) const;
   };
 
-  inline SatSolver(PropEngine* propEngine, TheoryEngine* theoryEngine, context::Context* context,
-            const Options* options);
+  inline SatSolver(PropEngine* propEngine,
+                   TheoryEngine* theoryEngine,
+                   context::Context* context,
+                   const Options* options);
 
   inline ~SatSolver();
 
@@ -93,8 +95,6 @@ public:
   inline void enqueueTheoryLiteral(const SatLiteral& l);
 
   inline void setCnfStream(CnfStream* cnfStream);
-
-  inline void clearAssertionQueues();
 };
 
 }/* CVC4::prop namespace */
@@ -135,7 +135,8 @@ inline std::ostream& operator <<(std::ostream& out, const SatClause& clause) {
   return out;
 }
 
-size_t SatSolver::SatLiteralHashFcn::operator()(const SatLiteral& literal) const {
+inline size_t
+SatSolver::SatLiteralHashFcn::operator()(const SatLiteral& literal) const {
   return (size_t) minisat::toInt(literal);
 }
 
@@ -208,11 +209,6 @@ void SatSolver::setCnfStream(CnfStream* cnfStream) {
   d_cnfStream = cnfStream;
 }
 
-
-void SatSolver::clearAssertionQueues() {
-  d_theoryEngine->clearAssertionQueues();
-}
-
 }/* CVC4::prop namespace */
 }/* CVC4 namespace */
 
index e97adb1d23c2f38ff2fd14344097a406916afb9b..c00112cd05cc7cf225eea57ccb46e657b12b795f 100644 (file)
@@ -70,13 +70,23 @@ SmtEngine::SmtEngine(ExprManager* em, const Options* opts) throw () :
   d_options(opts) {
 
   NodeManagerScope nms(d_nodeManager);
+
   d_decisionEngine = new DecisionEngine;
   d_theoryEngine = new TheoryEngine(this, d_ctxt);
   d_propEngine = new PropEngine(opts, d_decisionEngine, d_theoryEngine, d_ctxt);
 }
 
+void SmtEngine::shutdown() {
+  d_propEngine->shutdown();
+  d_theoryEngine->shutdown();
+  d_decisionEngine->shutdown();
+}
+
 SmtEngine::~SmtEngine() {
   NodeManagerScope nms(d_nodeManager);
+
+  shutdown();
+
   delete d_propEngine;
   delete d_theoryEngine;
   delete d_decisionEngine;
index 36cb8746cbd90d6d39bc733e79b04983d7b8d0b7..4836b282ebeb275b17fbfa80374474e57fb8d094 100644 (file)
@@ -145,6 +145,14 @@ private:
   // now in an SmtEnginePrivate class.  See the comment in
   // smt_engine.cpp.
 
+  /**
+   * This is called by the destructor, just before destroying the
+   * PropEngine, TheoryEngine, and DecisionEngine (in that order).  It
+   * is important because there are destruction ordering issues
+   * between PropEngine and Theory.
+   */
+  void shutdown();
+
   /**
    * Full check of consistency in current context.  Returns true iff
    * consistent.
index fb9be454aac34a99162deea71d5c8165a554ef45..f77e6cdf1d5315c06a9767c1db64b80c80f36200 100644 (file)
 #include "theory/output_channel.h"
 #include "context/context.h"
 
-#include <queue>
+#include <deque>
 #include <list>
 
 #include <typeinfo>
 
 namespace CVC4 {
+
+class TheoryEngine;
+
 namespace theory {
 
 // rewrite cache support
@@ -50,13 +53,17 @@ class Theory {
 private:
 
   template <class T>
-  friend class TheoryImpl;
+  friend class ::CVC4::theory::TheoryImpl;
+
+  friend class ::CVC4::TheoryEngine;
 
   /**
    * Construct a Theory.
    */
   Theory(context::Context* ctxt, OutputChannel& out) throw() :
     d_context(ctxt),
+    d_facts(),
+    d_factsResetter(*this),
     d_out(&out) {
   }
 
@@ -65,12 +72,59 @@ private:
    */
   Theory();
 
+  /**
+   * The context for the Theory.
+   */
+  context::Context* d_context;
+
+  /**
+   * The assertFact() queue.
+   *
+   * This queue MUST be emptied by ANY call to check() at ANY effort
+   * level.  In debug builds, this is checked.  On backjump we clear
+   * the fact queue (see FactsResetter, below).
+   *
+   * These can safely be TNodes because the literal map maintained in
+   * the SAT solver keeps them live.  As an added benefit, if we have
+   * them as TNodes, dtors are cheap (optimized away?).
+   */
+  std::deque<TNode> d_facts;
+
+  /** Helper class to reset the fact queue on pop(). */
+  class FactsResetter : public context::ContextNotifyObj {
+    Theory& d_thy;
+
+  public:
+    FactsResetter(Theory& thy) :
+      context::ContextNotifyObj(thy.d_context),
+      d_thy(thy) {
+    }
+
+    void notify() {
+      d_thy.d_facts.clear();
+    }
+  } d_factsResetter;
+
+  friend class FactsResetter;
+
 protected:
 
   /**
-   * The output channel for the Theory.
+   * This is called at shutdown time by the TheoryEngine, just before
+   * destruction.  It is important because there are destruction
+   * ordering issues between PropEngine and Theory (based on what
+   * hard-links to Nodes are outstanding).  As the fact queue might be
+   * nonempty, we ensure here that it's clear.  If you overload this,
+   * you must make an explicit call here to the Theory implementation
+   * of this function too.
    */
-  context::Context* d_context;
+  virtual void shutdown() {
+    d_facts.clear();
+  }
+
+  context::Context* getContext() const {
+    return d_context;
+  }
 
   /**
    * The output channel for the Theory.
@@ -78,12 +132,11 @@ protected:
   OutputChannel* d_out;
 
   /**
-   * The assertFact() queue.
+   * Returns true if the assertFact queue is empty
    */
-  // FIXME CD: on backjump we clear the facts IFF the queue gets
-  // emptied on every DL.  In general I guess we need a CDQueue<>?
-  // Perhaps one that asserts it's empty at each push?
-  std::queue<Node> d_facts;
+  bool done() throw() {
+    return d_facts.empty();
+  }
 
   /**
    * Return whether a node is shared or not.  Used by setup().
@@ -91,10 +144,18 @@ protected:
   bool isShared(TNode n) throw();
 
   /**
-   * Returns true if the assertFact queue is empty
+   * Check whether a node is in the rewrite cache or not.
    */
-  bool done() throw() {
-    return d_facts.empty();
+  static bool inRewriteCache(TNode n) throw() {
+    return n.hasAttribute(RewriteCache());
+  }
+
+  /**
+   * Get the value of the rewrite cache (or Node::null()) if there is
+   * none).
+   */
+  static Node getRewriteCache(TNode n) throw() {
+    return n.getAttribute(RewriteCache());
   }
 
 public:
@@ -156,6 +217,13 @@ public:
    */
   virtual void preRegisterTerm(TNode) = 0;
 
+  /**
+   * Rewrite a term.  Done one time for a Node, ever.
+   */
+  virtual Node rewrite(TNode n) {
+    return n;
+  }
+
   /**
    * Register a term.
    *
@@ -174,19 +242,9 @@ public:
    */
   void assertFact(TNode n) {
     Debug("theory") << "Theory::assertFact(" << n << ")" << std::endl;
-    d_facts.push(n);
+    d_facts.push_back(n);
   }
 
-  /**
-   * Clear the assertion queue.
-   */
-  void clearAssertionQueue() {
-    while (d_facts.size() > 0) {
-      d_facts.pop();
-    }
-  }
-
-
   /**
    * Check the current assignment's consistency.
    */
@@ -306,7 +364,7 @@ Node TheoryImpl<T>::get() {
           "Theory::get() called with assertion queue empty!" );
 
   Node fact = d_facts.front();
-  d_facts.pop();
+  d_facts.pop_front();
 
   Debug("theory") << "Theory::get() => " << fact << "(" << d_facts.size() << " left)" << std::endl;
 
index 49e4c2a88aa8dd9637b290eb55e08ceb049c3538..2323a305be6a0cef9287cf57cfa2ed2734b55326 100644 (file)
@@ -85,4 +85,44 @@ Node TheoryEngine::preprocess(TNode t) {
   return top;
 }
 
+Node TheoryEngine::rewrite(TNode in) {
+  if(inRewriteCache(in)) {
+    return getRewriteCache(in);
+  }
+
+  if(in.getKind() == kind::VARIABLE ||
+     in.getKind() == kind::SKOLEM) {
+    return in;
+  }
+
+  /*
+    theory::Theory* thy = theoryOf(in);
+    if(thy == NULL) {
+    Node out = rewriteBuiltins(in);
+    setRewriteCache(in, out);
+    return in;
+    } else {
+    Node out = thy->rewrite(in);
+    setRewriteCache(in, out);
+    return out;
+    }
+  */
+
+  if(in.getKind() == kind::EQUAL) {
+    Assert(in.getNumChildren() == 2);
+    if(in[0] == in[1]) {
+      Node out = NodeManager::currentNM()->mkNode(kind::TRUE);
+      //setRewriteCache(in, out);
+      return out;
+    }
+  } else {
+    Node out = rewriteChildren(in);
+    //setRewriteCache(in, out);
+    return out;
+  }
+
+  //setRewriteCache(in, in);
+  return in;
+}
+
 }/* CVC4 namespace */
index 4d3a3c296d3b5fa0f5b6a79ead9e469c2260596b..0b1afe748305ccbe3bc8c92a11a95c5e8a6b4f2b 100644 (file)
@@ -96,7 +96,7 @@ class TheoryEngine {
   /**
    * Check whether a node is in the rewrite cache or not.
    */
-  bool inRewriteCache(TNode n) throw() {
+  static bool inRewriteCache(TNode n) throw() {
     return n.hasAttribute(theory::RewriteCache());
   }
 
@@ -104,39 +104,58 @@ class TheoryEngine {
    * Get the value of the rewrite cache (or Node::null()) if there is
    * none).
    */
-  Node getRewriteCache(TNode n) throw() {
+  static Node getRewriteCache(TNode n) throw() {
     return n.getAttribute(theory::RewriteCache());
   }
 
-  Node rewrite(TNode in) {
-    /*
-    Node out = theoryOf(in)->rewrite(in);
-    in.setAttribute(theory::RewriteCache(), out);
-    return out;
-    */
-    if(inRewriteCache(in)) {
-      return getRewriteCache(in);
-    } else if(in.getKind() == kind::VARIABLE) {
-      return in;
-    } else if(in.getKind() == kind::EQUAL) {
-      Assert(in.getNumChildren() == 2);
-      if(in[0] == in[1]) {
-        Node out = NodeManager::currentNM()->mkNode(kind::TRUE);
-        //in.setAttribute(theory::RewriteCache(), out);
-        return out;
-      }
-    } else {
-      NodeBuilder<> b(in.getKind());
-      for(TNode::iterator c = in.begin(); c != in.end(); ++c) {
-        b << rewrite(*c);
-      }
-      Node out = b;
-      //in.setAttribute(theory::RewriteCache(), out);
-      return out;
+  /**
+   * Get the value of the rewrite cache (or Node::null()) if there is
+   * none).
+   */
+  static void setRewriteCache(TNode n, TNode v) throw() {
+    return n.setAttribute(theory::RewriteCache(), v);
+  }
+
+  /**
+   * This is the top rewrite entry point, called during preprocessing.
+   * It dispatches to the proper theories to rewrite the given Node.
+   */
+  Node rewrite(TNode in);
+
+  /**
+   * Convenience function to recurse through the children, rewriting,
+   * while leaving the Node's kind alone.
+   */
+  Node rewriteChildren(TNode in) {
+    NodeBuilder<> b(in.getKind());
+    for(TNode::iterator c = in.begin(); c != in.end(); ++c) {
+      b << rewrite(*c);
     }
+    return Node(b);
+  }
 
-    //in.setAttribute(theory::RewriteCache(), in);
-    return in;
+  /**
+   * Rewrite Nodes with builtin kind (that is, those Nodes n for which
+   * theoryOf(n) == NULL).  The master list is in expr/builtin_kinds.
+   */
+  Node rewriteBuiltins(TNode in) {
+    switch(Kind k = in.getKind()) {
+    case kind::EQUAL:
+      return rewriteChildren(in);
+
+    case kind::ITE:
+      Unhandled(k);
+
+    case kind::SKOLEM:
+    case kind::VARIABLE:
+      return in;
+
+    case kind::TUPLE:
+      return rewriteChildren(in);
+
+    default:
+      Unhandled(k);
+    }
   }
 
 public:
@@ -160,6 +179,19 @@ public:
     d_theoryOfTable.registerTheory(&d_bv);
   }
 
+  /**
+   * This is called at shutdown time by the SmtEngine, just before
+   * destruction.  It is important because there are destruction
+   * ordering issues between PropEngine and Theory.
+   */
+  void shutdown() {
+    d_bool.shutdown();
+    d_uf.shutdown();
+    d_arith.shutdown();
+    d_arrays.shutdown();
+    d_bv.shutdown();
+  }
+
   /**
    * Get the theory associated to a given Node.
    *
@@ -241,13 +273,6 @@ public:
     return d_theoryOut.d_conflictNode;
   }
 
-  /**
-   * Clears the queues of the theories.
-   */
-  void clearAssertionQueues() {
-    d_uf.clearAssertionQueue();
-  }
-
 };/* class TheoryEngine */
 
 }/* CVC4 namespace */
index d6e1be9f2662ded24850fe8389d97776ac3c6411..6d949d6de2a66197a72be483805646eea54f0f04 100644 (file)
@@ -103,7 +103,7 @@ void TheoryUF::registerTerm(TNode n){
            "during backtracking");
   }else{
     //The attribute does not exist, so it is created and set
-    ecN = new (true) ECData(d_context, n);
+    ecN = new (true) ECData(getContext(), n);
     n.setAttribute(ECAttr(), ecN);
   }
 
@@ -134,7 +134,7 @@ void TheoryUF::registerTerm(TNode n){
         }
       }
 
-      ecChild->addPredecessor(n, d_context);
+      ecChild->addPredecessor(n, getContext());
     }
   }
   Debug("uf") << "uf: end registerTerm(" << n << ")" << std::endl;
index 9b6101a2a6ac3f0fb753bf0ee19aa9b019c994e6..06ea283a866a87f2343c06a6f38b731f9ed19c43 100644 (file)
@@ -26,7 +26,7 @@ DecisionEngine::~DecisionEngine() {
  * virtual in the final design (?)
  */
 Node DecisionEngine::nextDecision() {
-  Unreachable();
+  Unimplemented();
 }
 
 }/* CVC4 namespace */
index ac9fc5ffd755c6554a6d13eb50a70f1fb3ed0315..fd757b734f1a1c48de2853c1087754f5b47967d4 100644 (file)
@@ -41,6 +41,15 @@ public:
    */
   virtual Node nextDecision();// = 0
 
+  /**
+   * This is called by SmtEngine, at shutdown time, just before
+   * destruction.  It is important because there are destruction
+   * ordering issues between some parts of the system.  For now,
+   * there's nothing to do here in the DecisionEngine.
+   */
+  virtual void shutdown() {
+  }
+
   // TODO: design decision: decision engine should be notified of
   // propagated lits, and also why(?) (so that it can make decisions
   // based on the utility of various theories and various theory