From 8d68b9ace8753a9280452b33175caef396bf2023 Mon Sep 17 00:00:00 2001 From: Kris Coester Date: Mon, 14 Oct 2024 11:21:53 +0200 Subject: [PATCH 01/14] Improve exception safety during clone --- gecode/kernel/core.cpp | 100 +++++++++++++++++++------------------- gecode/kernel/core.hpp | 84 +++++++++++++++++++++++++++++--- gecode/kernel/var-imp.hpp | 41 ++++++++++++++++ gecode/kernel/var.hpp | 1 + 4 files changed, 168 insertions(+), 58 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index 61aae361c7..701a0743af 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -194,7 +194,7 @@ namespace Gecode { d_fst = NULL; while (a < e) { // Ignore entries for tracers - if (!Support::marked(*a)) + if (!Support::marked(*a) && dynamic_cast(*a) != nullptr) (void) (*a)->dispose(*this); a++; } @@ -701,48 +701,56 @@ namespace Gecode { for (int i=0; inext(); a != e; a = a->next()) { - Actor* c = Actor::cast(a)->copy(*this); - // Link copied actor - p->next(ActorLink::cast(c)); ActorLink::cast(c)->prev(p); - // Note that forwarding is done in the constructors - p = c; + try{ + for (int i=0; inext(); a != e; a = a->next()) { + Actor* c = Actor::cast(a)->copy(*this); + // Link copied actor + p->next(ActorLink::cast(c)); ActorLink::cast(c)->prev(p); + // Note that forwarding is done in the constructors + p = c; + } + // Link last actor + p->next(&pl); pl.prev(p); } - // Link last actor - p->next(&pl); pl.prev(p); - } - // Copy all branchers - { - ActorLink* p = &bl; - ActorLink* e = &s.bl; - for (ActorLink* a = e->next(); a != e; a = a->next()) { - Actor* c = Actor::cast(a)->copy(*this); - // Link copied actor - p->next(ActorLink::cast(c)); ActorLink::cast(c)->prev(p); - // Note that forwarding is done in the constructors - p = c; + // Copy all branchers + { + ActorLink* p = &bl; + ActorLink* e = &s.bl; + for (ActorLink* a = e->next(); a != e; a = a->next()) { + Actor* c = Actor::cast(a)->copy(*this); + // Link copied actor + p->next(ActorLink::cast(c)); ActorLink::cast(c)->prev(p); + // Note that forwarding is done in the constructors + p = c; + } + // Link last actor + p->next(&bl); bl.prev(p); + } + // Setup brancher pointers + if (s.b_status == &s.bl) { + b_status = Brancher::cast(&bl); + } else { + b_status = Brancher::cast(s.b_status->prev()); + } + if (s.b_commit == &s.bl) { + b_commit = Brancher::cast(&bl); + } else { + b_commit = Brancher::cast(s.b_commit->prev()); } - // Link last actor - p->next(&bl); bl.prev(p); - } - // Setup brancher pointers - if (s.b_status == &s.bl) { - b_status = Brancher::cast(&bl); - } else { - b_status = Brancher::cast(s.b_status->prev()); } - if (s.b_commit == &s.bl) { - b_commit = Brancher::cast(&bl); - } else { - b_commit = Brancher::cast(s.b_commit->prev()); + catch(...) + { + recover(static_cast(mm.subscriptions())); + mm.release(ssd.data().sm); + throw; } } @@ -781,15 +789,8 @@ namespace Gecode { } // Update variables without indexing structure - VarImp* x = - static_cast*>(c->pc.c.vars_noidx); - while (x != NULL) { - VarImp* n = x->next(); - x->b.base = NULL; x->u.idx[0] = 0; - if (sizeof(ActorLink**) > sizeof(unsigned int)) - *(1+&x->u.idx[0]) = 0; - x = n; - } + updateNoIdx(c, false); //on space copy + // Update variables with indexing structure c->update(static_cast(c->mm.subscriptions())); @@ -864,7 +865,6 @@ namespace Gecode { return true; } - void Space::afc_unshare(void) { if (ssd.data().gpi.unshare()) { diff --git a/gecode/kernel/core.hpp b/gecode/kernel/core.hpp index 9d5f73ad5d..ddd649b022 100755 --- a/gecode/kernel/core.hpp +++ b/gecode/kernel/core.hpp @@ -299,6 +299,9 @@ namespace Gecode { */ static void update(Space& home, ActorLink**& sub); + /// Recover variables after OOM situations + static void recover(Space& home, ActorLink**& sub); + /// Enter propagator to subscription array void enter(Space& home, Propagator* p, PropCond pc); /// Enter advisor to subscription array @@ -1025,7 +1028,7 @@ namespace Gecode { /// Return alternative unsigned int alternative(void) const; }; - + /** * \brief Post trace information */ @@ -1872,6 +1875,11 @@ namespace Gecode { void update(ActorLink** sub); //@} + /// Update variables without indexing structure + void updateNoIdx(Space* space, bool recover); + + void recover(ActorLink** sub); + /// First actor for forced disposal Actor** d_fst; /// Current actor for forced disposal @@ -4244,13 +4252,22 @@ namespace Gecode { : var_id(x.var_id) #endif { +#ifndef NDEBUG + std::cout << "Generate in space " << &home << " in var-imp copy constructor " << this << " from x " << &x << std::endl; +#endif VarImpBase** reg; free_and_bits = x.free_and_bits & ((1 << free_bits) - 1); if (x.b.base == NULL) { // Variable implementation needs no index structure +#ifndef NDEBUG + std::cout << "x is not an indexed var " << std::endl; +#endif reg = &home.pc.c.vars_noidx; assert(x.degree() == 0); } else { +#ifndef NDEBUG + std::cout << "x is indexed var " << std::endl; +#endif reg = &home.pc.c.vars_u[idx_c]; } // Save subscriptions in copy @@ -4261,6 +4278,9 @@ namespace Gecode { // Set forwarding pointer x.b.fwd = static_cast*>(Support::mark(this)); +#ifndef NDEBUG + std::cout << "marked x as copied" << std::endl; +#endif // Register original x.u.next = static_cast*>(*reg); *reg = &x; } @@ -4431,6 +4451,10 @@ namespace Gecode { while (*f != a) f++; #endif // Remove actor +#ifndef NDEBUG + std::cout << "trying to remove actor" << std::endl; + std::cout << "current varimp " << this << " with base being " << b.base << std::endl; +#endif *f = *(actorNonZero(pc+1)-1); for (PropCond j = pc+1; j< pc_max+1; j++) { *(actorNonZero(j)-1) = *(actorNonZero(j+1)-1); @@ -4568,7 +4592,7 @@ namespace Gecode { template ModEvent VarImp::fail(Space& home) { - _fail(home); + _fail(home); return ME_GEN_FAILED; } @@ -4578,6 +4602,10 @@ namespace Gecode { // this refers to the variable to be updated (clone) // x refers to the original // Recover from copy +#ifndef NDEBUG + std::cout << "type in update is " << typeid(VarImp).name() << std::endl; + std::cout << "construct update, update original x was " << x << " and in storage recovered base is " << b.base << std::endl; +#endif x->b.base = b.base; x->u.idx[0] = u.idx[0]; if (pc_max > 0 && sizeof(ActorLink**) > sizeof(unsigned int)) @@ -4585,8 +4613,8 @@ namespace Gecode { unsigned int np = static_cast(x->actorNonZero(pc_max+1) - x->actor(0)); - unsigned int na = - static_cast(x->b.base + x->entries - + unsigned int na = + static_cast(x->b.base + x->entries - x->actorNonZero(pc_max+1)); unsigned int n = na + np; assert(n == x->degree()); @@ -4596,12 +4624,15 @@ namespace Gecode { sub += n; b.base = t; +#ifndef NDEBUG + std::cout << "constructed new x " << this << " with base = " << b.base << std::endl; +#endif // Process propagator subscriptions while (np >= 4) { ActorLink* p3 = f[3]->prev(); ActorLink* p0 = f[0]->prev(); ActorLink* p1 = f[1]->prev(); - ActorLink* p2 = f[2]->prev(); + ActorLink* p2 = f[2]->prev(); t[0] = p0; t[1] = p1; t[2] = p2; t[3] = p3; np -= 4; t += 4; f += 4; } @@ -4621,7 +4652,7 @@ namespace Gecode { ptrdiff_t m0, m1, m2, m3; ActorLink* p3 = static_cast(Support::ptrsplit(f[3],m3))->prev(); - ActorLink* p0 = + ActorLink* p0 = static_cast(Support::ptrsplit(f[0],m0))->prev(); ActorLink* p1 = static_cast(Support::ptrsplit(f[1],m1))->prev(); @@ -4635,7 +4666,7 @@ namespace Gecode { } if (na >= 2) { ptrdiff_t m0, m1; - ActorLink* p0 = + ActorLink* p0 = static_cast(Support::ptrsplit(f[0],m0))->prev(); ActorLink* p1 = static_cast(Support::ptrsplit(f[1],m1))->prev(); @@ -4645,7 +4676,7 @@ namespace Gecode { } if (na > 0) { ptrdiff_t m0; - ActorLink* p0 = + ActorLink* p0 = static_cast(Support::ptrsplit(f[0],m0))->prev(); t[0] = static_cast(Support::ptrjoin(p0,m0)); } @@ -4654,12 +4685,49 @@ namespace Gecode { template forceinline void VarImp::update(Space& home, ActorLink**& sub) { +#ifndef NDEBUG + std::cout << "update space " << &home << std::endl; +#endif VarImp* x = static_cast*>(home.pc.c.vars_u[idx_c]); while (x != NULL) { VarImp* n = x->next(); x->forward()->update(x,sub); x = n; } } + template + forceinline void + VarImp::recover(Space& home, ActorLink**& sub) { + VarImp* x = static_cast*>(home.pc.c.vars_u[idx_c]); +#ifndef NDEBUG + std::cout << "type in recover is " << typeid(VarImp).name() << std::endl; + std::cout << "trying to recover for idx_c " << idx_c << " via space " << &home << " the original variable " << x << std::endl; +#endif + while (x != NULL) { + if(x->copied()) + { + VarImp* n = x->next(); + VarImp* copy = x->forward(); +#ifndef NDEBUG + std::cout << "recover x" << std::endl; + std::cout << "was copied, x is " << x << " copy pointer is " << copy << std::endl; + std::cout << "set.base from " << x->b.base << " to " << copy->b.base << std::endl; +#endif + x->b.base = copy->b.base; + x->u.idx[0] = copy->u.idx[0]; + if (pc_max > 0 && sizeof(ActorLink**) > sizeof(unsigned int)) + x->u.idx[1] = copy->u.idx[1]; + x = n; + std::cout << "n = " << n << std::endl; + } + else { +#ifndef NDEBUG + std::cout << "nope, x = " << x << " was not copied" << std::endl; +#endif + break; + } + } + } + /* diff --git a/gecode/kernel/var-imp.hpp b/gecode/kernel/var-imp.hpp index a5c26981eb..a2ab562f17 100644 --- a/gecode/kernel/var-imp.hpp +++ b/gecode/kernel/var-imp.hpp @@ -493,5 +493,46 @@ namespace Gecode { Gecode::VarImp::update(*this,sub); #endif } + + forceinline void Space::updateNoIdx(Space* space, bool recover) + { + + VarImp* x = + static_cast*>(space->pc.c.vars_noidx); + while (x != NULL) { +#ifndef NDEBUG + std::cout << (recover? "recover": "update") << + "variable without indexing structure " << x << std::endl; + std::cout << "x is copied?" << x->copied() << std::endl; +#endif + VarImp* n = x->next(); + x->b.base = NULL; x->u.idx[0] = 0; + if (sizeof(ActorLink**) > sizeof(unsigned int)) + *(1+&x->u.idx[0]) = 0; + x = n; + } + } + + // void recover(VarImp* x, ActorLink**& sub); + forceinline void + Space::recover(ActorLink** sub) + { + //recover non-indexed variables + Space::updateNoIdx(this, true); // recover current space +#ifdef GECODE_HAS_INT_VARS + Gecode::VarImp::recover(*this,sub); +#endif +#ifdef GECODE_HAS_INT_VARS + Gecode::VarImp::recover(*this,sub); +#endif +#ifdef GECODE_HAS_SET_VARS + Gecode::VarImp::recover(*this,sub); +#endif +#ifdef GECODE_HAS_FLOAT_VARS + Gecode::VarImp::recover(*this,sub); +#endif + } + + } // STATISTICS: kernel-var diff --git a/gecode/kernel/var.hpp b/gecode/kernel/var.hpp index fec17ff1e9..29e10a0178 100755 --- a/gecode/kernel/var.hpp +++ b/gecode/kernel/var.hpp @@ -75,6 +75,7 @@ namespace Gecode { //@{ /// Update this variable to be a clone of variable \a y void update(Space& home, VarImpVar& y); + //@} }; From 94007770b0e7d46909e679087285a6e7bfa6ab4e Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Thu, 21 Nov 2024 11:23:27 +0100 Subject: [PATCH 02/14] Initialize members in Space copy which are used in Space dtor --- gecode/kernel/core.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index 701a0743af..68201f10e3 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -696,7 +696,7 @@ namespace Gecode { #ifdef GECODE_HAS_CBS var_id_counter(s.var_id_counter), #endif - d_fst(&Actor::sentinel) { + d_fst(&Actor::sentinel),d_cur(nullptr),d_lst(nullptr){ #ifdef GECODE_HAS_VAR_DISPOSE for (int i=0; i Date: Mon, 25 Nov 2024 14:03:05 +0100 Subject: [PATCH 03/14] Clean-up & provide productive gecode version --- gecode/kernel/core.cpp | 7 ++++--- gecode/kernel/core.hpp | 1 + gecode/kernel/var-imp.hpp | 7 ------- gecode/kernel/var.hpp | 1 - 4 files changed, 5 insertions(+), 11 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index 68201f10e3..f838a1c711 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -194,7 +194,7 @@ namespace Gecode { d_fst = NULL; while (a < e) { // Ignore entries for tracers - if (!Support::marked(*a) && dynamic_cast(*a) != nullptr) + if (!Support::marked(*a)) (void) (*a)->dispose(*this); a++; } @@ -696,7 +696,7 @@ namespace Gecode { #ifdef GECODE_HAS_CBS var_id_counter(s.var_id_counter), #endif - d_fst(&Actor::sentinel),d_cur(nullptr),d_lst(nullptr){ + d_fst(&Actor::sentinel),d_cur(nullptr),d_lst(nullptr) { #ifdef GECODE_HAS_VAR_DISPOSE for (int i=0; iprev()); } } - catch(...) + catch(const std::exception&) { recover(static_cast(mm.subscriptions())); mm.release(ssd.data().sm); @@ -865,6 +865,7 @@ namespace Gecode { return true; } + void Space::afc_unshare(void) { if (ssd.data().gpi.unshare()) { diff --git a/gecode/kernel/core.hpp b/gecode/kernel/core.hpp index ddd649b022..e726a79845 100755 --- a/gecode/kernel/core.hpp +++ b/gecode/kernel/core.hpp @@ -1878,6 +1878,7 @@ namespace Gecode { /// Update variables without indexing structure void updateNoIdx(Space* space, bool recover); + /// Recover after OOM situations void recover(ActorLink** sub); /// First actor for forced disposal diff --git a/gecode/kernel/var-imp.hpp b/gecode/kernel/var-imp.hpp index a2ab562f17..04f717c59e 100644 --- a/gecode/kernel/var-imp.hpp +++ b/gecode/kernel/var-imp.hpp @@ -496,15 +496,9 @@ namespace Gecode { forceinline void Space::updateNoIdx(Space* space, bool recover) { - VarImp* x = static_cast*>(space->pc.c.vars_noidx); while (x != NULL) { -#ifndef NDEBUG - std::cout << (recover? "recover": "update") << - "variable without indexing structure " << x << std::endl; - std::cout << "x is copied?" << x->copied() << std::endl; -#endif VarImp* n = x->next(); x->b.base = NULL; x->u.idx[0] = 0; if (sizeof(ActorLink**) > sizeof(unsigned int)) @@ -513,7 +507,6 @@ namespace Gecode { } } - // void recover(VarImp* x, ActorLink**& sub); forceinline void Space::recover(ActorLink** sub) { diff --git a/gecode/kernel/var.hpp b/gecode/kernel/var.hpp index 29e10a0178..fec17ff1e9 100755 --- a/gecode/kernel/var.hpp +++ b/gecode/kernel/var.hpp @@ -75,7 +75,6 @@ namespace Gecode { //@{ /// Update this variable to be a clone of variable \a y void update(Space& home, VarImpVar& y); - //@} }; From 6c34b3c0bc291bacd502f1f2ea77b795ad7ec9f3 Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Mon, 10 Feb 2025 11:18:17 +0100 Subject: [PATCH 04/14] Introduce & use isFullyConstructed variable; refactor & cleanup --- gecode/kernel/core.cpp | 42 +++++++++++++++++------- gecode/kernel/core.hpp | 68 ++++++++++++--------------------------- gecode/kernel/var-imp.hpp | 27 +++++++++++++--- 3 files changed, 72 insertions(+), 65 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index f838a1c711..a819834c85 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -112,7 +112,7 @@ namespace Gecode { VarImpDisposerBase* Space::vd[AllVarConf::idx_d]; #endif - Space::Space(void) : mm(ssd.data().sm) { + Space::Space(void) : mm(ssd.data().sm), isFullyConstructed(false) { #ifdef GECODE_HAS_CBS var_id_counter = 0; #endif @@ -134,6 +134,7 @@ namespace Gecode { pc.p.bid_sc = (reserved_bid+1) << sc_bits; pc.p.n_sub = 0; pc.p.vti.other(); + isFullyConstructed = true; } void @@ -166,7 +167,7 @@ namespace Gecode { void Space::ap_ignore_dispose(Actor* a, bool duplicate) { // Note that a might be a marked pointer! - assert(d_fst != NULL); + assert(d_fst != NULL || inPrematureDestructionMode()); Actor** f = d_fst; if (duplicate) { while (f < d_cur) @@ -693,6 +694,7 @@ namespace Gecode { Space::Space(Space& s) : ssd(s.ssd), mm(ssd.data().sm,s.mm,s.pc.p.n_sub*sizeof(Propagator**)), + isFullyConstructed(false), #ifdef GECODE_HAS_CBS var_id_counter(s.var_id_counter), #endif @@ -748,7 +750,14 @@ namespace Gecode { } catch(const std::exception&) { - recover(static_cast(mm.subscriptions())); + recover(s); + mm.release(ssd.data().sm); + throw; + } + catch(...) + { + std::cout << "Caught non-std exception in copy ctor, recover"; + recover(s); mm.release(ssd.data().sm); throw; } @@ -763,6 +772,7 @@ namespace Gecode { // Copy all data structures (which in turn will invoke the constructor) Space* c = copy(); + c->isFullyConstructed = false; if (c->d_fst != &Actor::sentinel) throw SpaceNotCloned("Space::clone"); @@ -775,15 +785,22 @@ namespace Gecode { c->d_fst = c->d_cur = c->d_lst = NULL; } else { // Leave one entry free - c->d_fst = c->alloc(n+1); - c->d_cur = c->d_fst; - c->d_lst = c->d_fst+n+1; - for (Actor** d_fst_iter = d_fst; d_fst_iter != d_cur; d_fst_iter++) { - ptrdiff_t m; - Actor* a = static_cast(Support::ptrsplit(*d_fst_iter,m)); - if (a->prev()) - *(c->d_cur++) = Actor::cast(static_cast - (Support::ptrjoin(a->prev(),m))); + try{ + c->d_fst = c->alloc(n+1); + c->d_cur = c->d_fst; + c->d_lst = c->d_fst+n+1; + for (Actor** d_fst_iter = d_fst; d_fst_iter != d_cur; d_fst_iter++) { + ptrdiff_t m; + Actor* a = static_cast(Support::ptrsplit(*d_fst_iter,m)); + if (a->prev()) + *(c->d_cur++) = Actor::cast(static_cast + (Support::ptrjoin(a->prev(),m))); + } + } + catch(const std::exception&) + { + c->recover(*this); + throw; } } } @@ -835,6 +852,7 @@ namespace Gecode { // Reset execution information c->pc.p.vti.other(); pc.p.vti.other(); + c->isFullyConstructed = true; return c; } diff --git a/gecode/kernel/core.hpp b/gecode/kernel/core.hpp index e726a79845..836b306a0c 100755 --- a/gecode/kernel/core.hpp +++ b/gecode/kernel/core.hpp @@ -300,7 +300,7 @@ namespace Gecode { static void update(Space& home, ActorLink**& sub); /// Recover variables after OOM situations - static void recover(Space& home, ActorLink**& sub); + static void revertHarmfulChangesOfUnfinishedClone(Space& home, ActorLink**& sub); /// Enter propagator to subscription array void enter(Space& home, Propagator* p, PropCond pc); @@ -1855,6 +1855,9 @@ namespace Gecode { LocalObject* local; } c; } pc; + + bool isFullyConstructed; + /// Put propagator \a p into right queue void enqueue(Propagator* p); /** @@ -1878,9 +1881,6 @@ namespace Gecode { /// Update variables without indexing structure void updateNoIdx(Space* space, bool recover); - /// Recover after OOM situations - void recover(ActorLink** sub); - /// First actor for forced disposal Actor** d_fst; /// Current actor for forced disposal @@ -2001,7 +2001,19 @@ namespace Gecode { */ GECODE_KERNEL_EXPORT void ap_ignore_dispose(Actor* a, bool d); + + protected: + /// Recover after OOM situations + GECODE_KERNEL_EXPORT + void recover(Space& source); + public: + GECODE_KERNEL_EXPORT + bool inPrematureDestructionMode() + { + return !isFullyConstructed; + }; + /** * \brief Default constructor * \ingroup TaskModelScript @@ -4253,22 +4265,13 @@ namespace Gecode { : var_id(x.var_id) #endif { -#ifndef NDEBUG - std::cout << "Generate in space " << &home << " in var-imp copy constructor " << this << " from x " << &x << std::endl; -#endif VarImpBase** reg; free_and_bits = x.free_and_bits & ((1 << free_bits) - 1); if (x.b.base == NULL) { // Variable implementation needs no index structure -#ifndef NDEBUG - std::cout << "x is not an indexed var " << std::endl; -#endif reg = &home.pc.c.vars_noidx; assert(x.degree() == 0); } else { -#ifndef NDEBUG - std::cout << "x is indexed var " << std::endl; -#endif reg = &home.pc.c.vars_u[idx_c]; } // Save subscriptions in copy @@ -4279,9 +4282,6 @@ namespace Gecode { // Set forwarding pointer x.b.fwd = static_cast*>(Support::mark(this)); -#ifndef NDEBUG - std::cout << "marked x as copied" << std::endl; -#endif // Register original x.u.next = static_cast*>(*reg); *reg = &x; } @@ -4451,11 +4451,8 @@ namespace Gecode { #else while (*f != a) f++; #endif + // Remove actor -#ifndef NDEBUG - std::cout << "trying to remove actor" << std::endl; - std::cout << "current varimp " << this << " with base being " << b.base << std::endl; -#endif *f = *(actorNonZero(pc+1)-1); for (PropCond j = pc+1; j< pc_max+1; j++) { *(actorNonZero(j)-1) = *(actorNonZero(j+1)-1); @@ -4471,7 +4468,7 @@ namespace Gecode { template forceinline void VarImp::cancel(Space& home, Propagator& p, PropCond pc) { - if (b.base != NULL) + if (b.base != NULL && !home.inPrematureDestructionMode()) remove(home,&p,pc); } @@ -4603,10 +4600,6 @@ namespace Gecode { // this refers to the variable to be updated (clone) // x refers to the original // Recover from copy -#ifndef NDEBUG - std::cout << "type in update is " << typeid(VarImp).name() << std::endl; - std::cout << "construct update, update original x was " << x << " and in storage recovered base is " << b.base << std::endl; -#endif x->b.base = b.base; x->u.idx[0] = u.idx[0]; if (pc_max > 0 && sizeof(ActorLink**) > sizeof(unsigned int)) @@ -4625,9 +4618,6 @@ namespace Gecode { sub += n; b.base = t; -#ifndef NDEBUG - std::cout << "constructed new x " << this << " with base = " << b.base << std::endl; -#endif // Process propagator subscriptions while (np >= 4) { ActorLink* p3 = f[3]->prev(); @@ -4686,9 +4676,6 @@ namespace Gecode { template forceinline void VarImp::update(Space& home, ActorLink**& sub) { -#ifndef NDEBUG - std::cout << "update space " << &home << std::endl; -#endif VarImp* x = static_cast*>(home.pc.c.vars_u[idx_c]); while (x != NULL) { VarImp* n = x->next(); x->forward()->update(x,sub); x = n; @@ -4697,40 +4684,25 @@ namespace Gecode { template forceinline void - VarImp::recover(Space& home, ActorLink**& sub) { + VarImp::revertHarmfulChangesOfUnfinishedClone(Space& home, ActorLink**& sub) { VarImp* x = static_cast*>(home.pc.c.vars_u[idx_c]); -#ifndef NDEBUG - std::cout << "type in recover is " << typeid(VarImp).name() << std::endl; - std::cout << "trying to recover for idx_c " << idx_c << " via space " << &home << " the original variable " << x << std::endl; -#endif while (x != NULL) { if(x->copied()) { VarImp* n = x->next(); VarImp* copy = x->forward(); -#ifndef NDEBUG - std::cout << "recover x" << std::endl; - std::cout << "was copied, x is " << x << " copy pointer is " << copy << std::endl; - std::cout << "set.base from " << x->b.base << " to " << copy->b.base << std::endl; -#endif x->b.base = copy->b.base; x->u.idx[0] = copy->u.idx[0]; if (pc_max > 0 && sizeof(ActorLink**) > sizeof(unsigned int)) x->u.idx[1] = copy->u.idx[1]; x = n; - std::cout << "n = " << n << std::endl; } - else { -#ifndef NDEBUG - std::cout << "nope, x = " << x << " was not copied" << std::endl; -#endif + else break; - } } } - /* * Variable disposer * diff --git a/gecode/kernel/var-imp.hpp b/gecode/kernel/var-imp.hpp index 04f717c59e..b3ebb12e57 100644 --- a/gecode/kernel/var-imp.hpp +++ b/gecode/kernel/var-imp.hpp @@ -508,21 +508,38 @@ namespace Gecode { } forceinline void - Space::recover(ActorLink** sub) + Space::recover(Space& source) { + //dispose the propagators of the (not fully) copied clone, since they are not registered for disposale + ActorLink* p_a = &(source.pl); + ActorLink* c_a = p_a->next(); + + //before being recovered loop over the source-propagators + //if the previous not the current previous, then it has to be a copied on, right + while (c_a != &(source.pl)) { + if(c_a->prev() != p_a) + { + assert(this->inPrematureDestructionMode()); + if (!Support::marked(Actor::cast(c_a->prev()))) + (void) Actor::cast(c_a->prev())->dispose(*this); + } + p_a = c_a; c_a = c_a->next(); + } + //recover non-indexed variables + ActorLink** sub = static_cast(mm.subscriptions()); Space::updateNoIdx(this, true); // recover current space #ifdef GECODE_HAS_INT_VARS - Gecode::VarImp::recover(*this,sub); + Gecode::VarImp::revertHarmfulChangesOfUnfinishedClone(*this,sub); #endif #ifdef GECODE_HAS_INT_VARS - Gecode::VarImp::recover(*this,sub); + Gecode::VarImp::revertHarmfulChangesOfUnfinishedClone(*this,sub); #endif #ifdef GECODE_HAS_SET_VARS - Gecode::VarImp::recover(*this,sub); + Gecode::VarImp::revertHarmfulChangesOfUnfinishedClone(*this,sub); #endif #ifdef GECODE_HAS_FLOAT_VARS - Gecode::VarImp::recover(*this,sub); + Gecode::VarImp::revertHarmfulChangesOfUnfinishedClone(*this,sub); #endif } From 6a89be24f2913625a83b41dbcac6841c45802a4a Mon Sep 17 00:00:00 2001 From: Kris Coester Date: Thu, 20 Feb 2025 11:02:30 +0100 Subject: [PATCH 05/14] Fix detected issues of recover implementation in case of OOM --- gecode/kernel/core.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index a819834c85..87e2921ddf 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -167,7 +167,14 @@ namespace Gecode { void Space::ap_ignore_dispose(Actor* a, bool duplicate) { // Note that a might be a marked pointer! - assert(d_fst != NULL || inPrematureDestructionMode()); + + if(inPrematureDestructionMode())//actor array is not set up yet + { + return; + } + + assert(d_fst != NULL); + Actor** f = d_fst; if (duplicate) { while (f < d_cur) @@ -756,7 +763,6 @@ namespace Gecode { } catch(...) { - std::cout << "Caught non-std exception in copy ctor, recover"; recover(s); mm.release(ssd.data().sm); throw; @@ -800,6 +806,7 @@ namespace Gecode { catch(const std::exception&) { c->recover(*this); + delete c; throw; } } From 80bf349e13d0865d1bef599048f01b5de1e2217a Mon Sep 17 00:00:00 2001 From: Kris Coester Date: Tue, 4 Mar 2025 12:43:25 +0100 Subject: [PATCH 06/14] Prevent cancellation of advisors from variables in premature destruction mode --- gecode/kernel/core.hpp | 4 ++-- gecode/kernel/var-imp.hpp | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gecode/kernel/core.hpp b/gecode/kernel/core.hpp index 836b306a0c..14c1e114b2 100755 --- a/gecode/kernel/core.hpp +++ b/gecode/kernel/core.hpp @@ -2005,7 +2005,7 @@ namespace Gecode { protected: /// Recover after OOM situations GECODE_KERNEL_EXPORT - void recover(Space& source); + virtual void recover(Space& source); public: GECODE_KERNEL_EXPORT @@ -4498,7 +4498,7 @@ namespace Gecode { template forceinline void VarImp::cancel(Space& home, Advisor& a, bool fail) { - if (b.base != NULL) { + if (b.base != NULL && !home.inPrematureDestructionMode()) { Advisor* ma = static_cast(Support::ptrjoin(&a,fail ? 1 : 0)); remove(home,ma); } diff --git a/gecode/kernel/var-imp.hpp b/gecode/kernel/var-imp.hpp index b3ebb12e57..4e640a90be 100644 --- a/gecode/kernel/var-imp.hpp +++ b/gecode/kernel/var-imp.hpp @@ -522,6 +522,7 @@ namespace Gecode { assert(this->inPrematureDestructionMode()); if (!Support::marked(Actor::cast(c_a->prev()))) (void) Actor::cast(c_a->prev())->dispose(*this); + c_a->prev(p_a); } p_a = c_a; c_a = c_a->next(); } From 124aeb2f2a1e33b4c4724c150524184aefc09c51 Mon Sep 17 00:00:00 2001 From: Kris Coester Date: Wed, 5 Mar 2025 11:23:33 +0100 Subject: [PATCH 07/14] Move increase of propagation in status statistics to virtual method --- gecode/kernel/core.cpp | 6 +++--- gecode/kernel/core.hpp | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index 87e2921ddf..b0142365a3 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -271,7 +271,7 @@ namespace Gecode { // Check whether space is stable but not failed goto f_unstable; f_execute: - stat.propagate++; + trackPropagation(stat, p); // Keep old modification event delta med_o = p->u.med; // Clear med but leave propagator in queue @@ -331,7 +331,7 @@ namespace Gecode { // Support for disabled propagators goto d_unstable; d_execute: - stat.propagate++; + trackPropagation(stat, p); if (p->disabled()) goto d_put_into_idle; // Keep old modification event delta @@ -408,7 +408,7 @@ namespace Gecode { goto t_unstable; t_execute: - stat.propagate++; + trackPropagation(stat, p); if (p->disabled()) goto t_put_into_idle; pc.p.vti.propagator(*p); diff --git a/gecode/kernel/core.hpp b/gecode/kernel/core.hpp index 14c1e114b2..28a841ad7a 100755 --- a/gecode/kernel/core.hpp +++ b/gecode/kernel/core.hpp @@ -2007,6 +2007,12 @@ namespace Gecode { GECODE_KERNEL_EXPORT virtual void recover(Space& source); + GECODE_KERNEL_EXPORT + virtual void trackPropagation(StatusStatistics& stat, const Propagator* /*p*/) + { + stat.propagate++; + } + public: GECODE_KERNEL_EXPORT bool inPrematureDestructionMode() From f47844cb404d3d0bfbe5264c1cbce21fc76ffadf Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Wed, 26 Mar 2025 14:29:01 +0100 Subject: [PATCH 08/14] Fix memory leak in IntSetObject::allocate --- gecode/int/int-set.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/gecode/int/int-set.cpp b/gecode/int/int-set.cpp index 50646eba7f..559e25dacb 100755 --- a/gecode/int/int-set.cpp +++ b/gecode/int/int-set.cpp @@ -39,7 +39,14 @@ namespace Gecode { IntSet::IntSetObject::allocate(int n) { IntSetObject* o = new IntSetObject; o->n = n; - o->r = heap.alloc(n); + try { + o->r = heap.alloc(n); + } + catch (const std::bad_alloc& ex) + { + delete(o); + throw ex; + } return o; } @@ -165,7 +172,7 @@ namespace Gecode { Region reg; Range* dr = reg.alloc(n); int j=0; - for (const std::pair& k : r) + for (const std::pair& k : r) if (k.first <= k.second) { dr[j].min=k.first; dr[j].max=k.second; j++; } From 69c03a9d30c84693f54a98d5148124c9ca5e5058 Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Thu, 27 Mar 2025 10:38:43 +0100 Subject: [PATCH 09/14] Fix memory corruption in IntSetObject::allocate --- gecode/int.hh | 38 ++++++++++++++++++++------------------ gecode/int/int-set.cpp | 3 +-- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/gecode/int.hh b/gecode/int.hh index d6e50317cd..a57bae0c2b 100755 --- a/gecode/int.hh +++ b/gecode/int.hh @@ -188,6 +188,8 @@ namespace Gecode { int n; /// Array of ranges Range* r; + //Constructor to initialize members + GECODE_INT_EXPORT IntSetObject(int n_): size(0u), n(n_), r(nullptr) {} /// Allocate object with \a m elements GECODE_INT_EXPORT static IntSetObject* allocate(int m); /// Check whether \a n is included in the set @@ -3285,7 +3287,7 @@ namespace Gecode { * * Schedule tasks with start times \a s and processing times \a p * on a unary resource. The propagator uses the algorithms from: - * Petr Vilím, Global Constraints in Scheduling, PhD thesis, + * Petr VilĂ­m, Global Constraints in Scheduling, PhD thesis, * Charles University, Prague, Czech Republic, 2007. * * The propagator performs propagation that depends on the integer @@ -3317,7 +3319,7 @@ namespace Gecode { * and whether a task is mandatory \a m (a task is mandatory if the * Boolean variable is 1) on a unary resource. The propagator uses the * algorithms from: - * Petr Vilím, Global Constraints in Scheduling, PhD thesis, + * Petr VilĂ­m, Global Constraints in Scheduling, PhD thesis, * Charles University, Prague, Czech Republic, 2007. * * The propagator performs propagation that depends on the integer @@ -3359,7 +3361,7 @@ namespace Gecode { * end time. * * The propagator uses the algorithms from: - * Petr Vilím, Global Constraints in Scheduling, PhD thesis, + * Petr VilĂ­m, Global Constraints in Scheduling, PhD thesis, * Charles University, Prague, Czech Republic, 2007. * * The propagator performs propagation that depends on the integer @@ -3401,7 +3403,7 @@ namespace Gecode { * * The propagator uses the * algorithms from: - * Petr Vilím, Global Constraints in Scheduling, PhD thesis, + * Petr VilĂ­m, Global Constraints in Scheduling, PhD thesis, * Charles University, Prague, Czech Republic, 2007. * * The propagator performs propagation that depends on the integer @@ -3431,7 +3433,7 @@ namespace Gecode { * Schedule tasks with start times \a s, processing times \a p, and * end times \a e * on a unary resource. The propagator uses the algorithms from: - * Petr Vilím, Global Constraints in Scheduling, PhD thesis, + * Petr VilĂ­m, Global Constraints in Scheduling, PhD thesis, * Charles University, Prague, Czech Republic, 2007. * * The propagator does not enforce \f$s_i+p_i=e_i\f$, this constraint @@ -3463,7 +3465,7 @@ namespace Gecode { * and whether a task is mandatory \a m (a task is mandatory if the * Boolean variable is 1) on a unary resource. The propagator uses the * algorithms from: - * Petr Vilím, Global Constraints in Scheduling, PhD thesis, + * Petr VilĂ­m, Global Constraints in Scheduling, PhD thesis, * Charles University, Prague, Czech Republic, 2007. * * The propagator performs propagation that depends on the integer @@ -3516,13 +3518,13 @@ namespace Gecode { * * The propagator uses algorithms taken from: * - * Petr Vilím, Max Energy Filtering Algorithm for Discrete Cumulative + * Petr VilĂ­m, Max Energy Filtering Algorithm for Discrete Cumulative * Resources, in W. J. van Hoeve and J. N. Hooker, editors, CPAIOR, volume * 5547 of LNCS, pages 294-308. Springer, 2009. * * and * - * Petr Vilím, Edge finding filtering algorithm for discrete cumulative + * Petr VilĂ­m, Edge finding filtering algorithm for discrete cumulative * resources in O(kn log n). In I. P. Gent, editor, CP, volume 5732 of LNCS, * pages 802-816. Springer, 2009. * @@ -3575,13 +3577,13 @@ namespace Gecode { * * The propagator uses algorithms taken from: * - * Petr Vilím, Max Energy Filtering Algorithm for Discrete Cumulative + * Petr VilĂ­m, Max Energy Filtering Algorithm for Discrete Cumulative * Resources, in W. J. van Hoeve and J. N. Hooker, editors, CPAIOR, volume * 5547 of LNCS, pages 294-308. Springer, 2009. * * and * - * Petr Vilím, Edge finding filtering algorithm for discrete cumulative + * Petr VilĂ­m, Edge finding filtering algorithm for discrete cumulative * resources in O(kn log n). In I. P. Gent, editor, CP, volume 5732 of LNCS, * pages 802-816. Springer, 2009. * @@ -3620,13 +3622,13 @@ namespace Gecode { * * The propagator uses algorithms taken from: * - * Petr Vilím, Max Energy Filtering Algorithm for Discrete Cumulative + * Petr VilĂ­m, Max Energy Filtering Algorithm for Discrete Cumulative * Resources, in W. J. van Hoeve and J. N. Hooker, editors, CPAIOR, volume * 5547 of LNCS, pages 294-308. Springer, 2009. * * and * - * Petr Vilím, Edge finding filtering algorithm for discrete cumulative + * Petr VilĂ­m, Edge finding filtering algorithm for discrete cumulative * resources in O(kn log n). In I. P. Gent, editor, CP, volume 5732 of LNCS, * pages 802-816. Springer, 2009. * @@ -3665,13 +3667,13 @@ namespace Gecode { * * The propagator uses algorithms taken from: * - * Petr Vilím, Max Energy Filtering Algorithm for Discrete Cumulative + * Petr VilĂ­m, Max Energy Filtering Algorithm for Discrete Cumulative * Resources, in W. J. van Hoeve and J. N. Hooker, editors, CPAIOR, volume * 5547 of LNCS, pages 294-308. Springer, 2009. * * and * - * Petr Vilím, Edge finding filtering algorithm for discrete cumulative + * Petr VilĂ­m, Edge finding filtering algorithm for discrete cumulative * resources in O(kn log n). In I. P. Gent, editor, CP, volume 5732 of LNCS, * pages 802-816. Springer, 2009. * @@ -3712,13 +3714,13 @@ namespace Gecode { * * The propagator uses algorithms taken from: * - * Petr Vilím, Max Energy Filtering Algorithm for Discrete Cumulative + * Petr VilĂ­m, Max Energy Filtering Algorithm for Discrete Cumulative * Resources, in W. J. van Hoeve and J. N. Hooker, editors, CPAIOR, volume * 5547 of LNCS, pages 294-308. Springer, 2009. * * and * - * Petr Vilím, Edge finding filtering algorithm for discrete cumulative + * Petr VilĂ­m, Edge finding filtering algorithm for discrete cumulative * resources in O(kn log n). In I. P. Gent, editor, CP, volume 5732 of LNCS, * pages 802-816. Springer, 2009. * @@ -3761,13 +3763,13 @@ namespace Gecode { * * The propagator uses algorithms taken from: * - * Petr Vilím, Max Energy Filtering Algorithm for Discrete Cumulative + * Petr VilĂ­m, Max Energy Filtering Algorithm for Discrete Cumulative * Resources, in W. J. van Hoeve and J. N. Hooker, editors, CPAIOR, volume * 5547 of LNCS, pages 294-308. Springer, 2009. * * and * - * Petr Vilím, Edge finding filtering algorithm for discrete cumulative + * Petr VilĂ­m, Edge finding filtering algorithm for discrete cumulative * resources in O(kn log n). In I. P. Gent, editor, CP, volume 5732 of LNCS, * pages 802-816. Springer, 2009. * diff --git a/gecode/int/int-set.cpp b/gecode/int/int-set.cpp index 559e25dacb..a210a57e64 100755 --- a/gecode/int/int-set.cpp +++ b/gecode/int/int-set.cpp @@ -37,8 +37,7 @@ namespace Gecode { IntSet::IntSetObject* IntSet::IntSetObject::allocate(int n) { - IntSetObject* o = new IntSetObject; - o->n = n; + IntSetObject* o = new IntSetObject(n); try { o->r = heap.alloc(n); } From 158ff185eb7cc85379ae30f6f608981c0131612d Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Mon, 7 Apr 2025 09:49:40 +0200 Subject: [PATCH 10/14] Fix exception handling in IntSetObject::allocate --- gecode/int/int-set.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gecode/int/int-set.cpp b/gecode/int/int-set.cpp index a210a57e64..a3b57744eb 100755 --- a/gecode/int/int-set.cpp +++ b/gecode/int/int-set.cpp @@ -44,7 +44,7 @@ namespace Gecode { catch (const std::bad_alloc& ex) { delete(o); - throw ex; + throw; } return o; } From 3de522083470f06cfb372e183239c6fb9d5a87a9 Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Tue, 8 Apr 2025 13:48:28 +0200 Subject: [PATCH 11/14] Fixed memory leak in Space::ap_notice_dispose --- gecode/kernel/core.cpp | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index b0142365a3..6158575028 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -149,14 +149,33 @@ namespace Gecode { // Resize if (d_fst == NULL) { // Create new array - d_fst = alloc(4); + try { + d_fst = alloc(4); + } + catch (const std::bad_alloc& e) + { + if (a != nullptr) + a->dispose(*this); + throw; + } + d_cur = d_fst; d_lst = d_fst+4; } else { // Resize existing array unsigned int n = static_cast(d_lst - d_fst); assert(n != 0); - d_fst = realloc(d_fst,n,2*n); + + try { + d_fst = realloc(d_fst,n,2*n); + } + catch (const std::bad_alloc&) + { + if (a != nullptr) + a->dispose(*this); + throw; + } + d_cur = d_fst+n; d_lst = d_fst+2*n; } From 197157bd54d826607d6731c83a44d9f482a87224 Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Thu, 17 Apr 2025 12:54:48 +0200 Subject: [PATCH 12/14] Fixed memory leak in BoolExpr & IntExpr Nodes --- gecode/minimodel/bool-expr.cpp | 11 +++++++- gecode/minimodel/int-expr.cpp | 48 ++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/gecode/minimodel/bool-expr.cpp b/gecode/minimodel/bool-expr.cpp index 91e02af4a5..8bbd094c5a 100755 --- a/gecode/minimodel/bool-expr.cpp +++ b/gecode/minimodel/bool-expr.cpp @@ -88,6 +88,7 @@ namespace Gecode { BoolExpr::Node::~Node(void) { delete m; + decrement(); } void* @@ -194,7 +195,15 @@ namespace Gecode { #endif BoolExpr::BoolExpr(BoolExpr::Misc* m) - : n(new Node) { + : n(nullptr) { + try { + n = new Node(); + } + catch (const std::bad_alloc&) + { + delete m; + throw; + } n->same = 1; n->t = NT_MISC; n->l = nullptr; diff --git a/gecode/minimodel/int-expr.cpp b/gecode/minimodel/int-expr.cpp index caf0e6bba0..93d3496b89 100755 --- a/gecode/minimodel/int-expr.cpp +++ b/gecode/minimodel/int-expr.cpp @@ -109,6 +109,7 @@ namespace Gecode { break; default: ; } + decrement(); } forceinline void* @@ -405,7 +406,14 @@ namespace Gecode { n->t = NT_SUM_INT; n->l = n->r = nullptr; if (x.size() > 0) { - n->sum.ti = heap.alloc >(x.size()); + try { + n->sum.ti = heap.alloc >(x.size()); + } + catch(const std::bad_alloc&) + { + delete n; + throw; + } for (int i=x.size(); i--; ) { n->sum.ti[i].x = x[i]; n->sum.ti[i].a = 1; @@ -422,7 +430,14 @@ namespace Gecode { n->t = NT_SUM_INT; n->l = n->r = nullptr; if (x.size() > 0) { - n->sum.ti = heap.alloc >(x.size()); + try { + n->sum.ti = heap.alloc >(x.size()); + } + catch (const std::bad_alloc&) + { + delete n; + throw; + } for (int i=x.size(); i--; ) { n->sum.ti[i].x = x[i]; n->sum.ti[i].a = a[i]; @@ -437,7 +452,15 @@ namespace Gecode { n->t = NT_SUM_BOOL; n->l = n->r = nullptr; if (x.size() > 0) { - n->sum.tb = heap.alloc >(x.size()); + try { + n->sum.tb = heap.alloc >(x.size()); + } + catch (const std::bad_alloc&) + { + delete n; + throw; + } + for (int i=x.size(); i--; ) { n->sum.tb[i].x = x[i]; n->sum.tb[i].a = 1; @@ -454,7 +477,14 @@ namespace Gecode { n->t = NT_SUM_BOOL; n->l = n->r = nullptr; if (x.size() > 0) { - n->sum.tb = heap.alloc >(x.size()); + try { + n->sum.tb = heap.alloc >(x.size()); + } + catch (const std::bad_alloc&) + { + delete n; + throw; + } for (int i=x.size(); i--; ) { n->sum.tb[i].x = x[i]; n->sum.tb[i].a = a[i]; @@ -492,7 +522,15 @@ namespace Gecode { } LinIntExpr::LinIntExpr(NonLinIntExpr* e) : - n(new Node) { + n(nullptr) { + try { + n = new Node; + } + catch (const std::bad_alloc&) + { + delete e; + throw; + } n->n_int = 1; n->n_bool = 0; n->t = NT_NONLIN; From 4eaeab9dba749441ffbf9221590b549b9e3e2cd5 Mon Sep 17 00:00:00 2001 From: Alexander Shepil Date: Mon, 12 May 2025 11:38:45 +0200 Subject: [PATCH 13/14] Fix leak in ArithNonLinIntExpr ctor --- gecode/minimodel/bool-expr.cpp | 1 - gecode/minimodel/int-arith.cpp | 15 ++++++++------- gecode/minimodel/int-expr.cpp | 14 ++++++-------- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/gecode/minimodel/bool-expr.cpp b/gecode/minimodel/bool-expr.cpp index 8bbd094c5a..d29739dc25 100755 --- a/gecode/minimodel/bool-expr.cpp +++ b/gecode/minimodel/bool-expr.cpp @@ -88,7 +88,6 @@ namespace Gecode { BoolExpr::Node::~Node(void) { delete m; - decrement(); } void* diff --git a/gecode/minimodel/int-arith.cpp b/gecode/minimodel/int-arith.cpp index 68674987b3..ac9340a8bd 100755 --- a/gecode/minimodel/int-arith.cpp +++ b/gecode/minimodel/int-arith.cpp @@ -53,23 +53,24 @@ namespace Gecode { namespace MiniModel { ANLE_ELMNT, ///< Element expression ANLE_ITE ///< If-then-else expression } t; - /// Expressions - LinIntExpr* a; + /// Boolean expression argument (used in ite for example) + BoolExpr b; /// Size of variable array int n; /// Integer argument (used in nroot for example) int aInt; - /// Boolean expression argument (used in ite for example) - BoolExpr b; + /// Expressions + LinIntExpr* a; + /// Constructor ArithNonLinIntExpr(ArithNonLinIntExprType t0, int n0) - : t(t0), a(heap.alloc(n0)), n(n0) {} + : t(t0), b(), n(n0), aInt(0), a(heap.alloc(n0)) {} // in case of OOM on a, dtor of b will be called /// Constructor ArithNonLinIntExpr(ArithNonLinIntExprType t0, int n0, int a0) - : t(t0), a(heap.alloc(n0)), n(n0), aInt(a0) {} + : t(t0), b(), n(n0), aInt(a0), a(heap.alloc(n0)) {} // ditto /// Constructor ArithNonLinIntExpr(ArithNonLinIntExprType t0, int n0, const BoolExpr& b0) - : t(t0), a(heap.alloc(n0)), n(n0), b(b0) {} + : t(t0), b(b0), n(n0), aInt(0), a(heap.alloc(n0)) {} // ditto /// Destructor ~ArithNonLinIntExpr(void) { heap.free(a,n); diff --git a/gecode/minimodel/int-expr.cpp b/gecode/minimodel/int-expr.cpp index 93d3496b89..1e2b5ac5d0 100755 --- a/gecode/minimodel/int-expr.cpp +++ b/gecode/minimodel/int-expr.cpp @@ -109,7 +109,6 @@ namespace Gecode { break; default: ; } - decrement(); } forceinline void* @@ -362,11 +361,10 @@ namespace Gecode { } LinIntExpr::LinIntExpr(void) : - n(new Node) { - n->n_int = n->n_bool = 0; - n->t = NT_VAR_INT; - n->l = n->r = nullptr; - n->a = 0; + n(nullptr) { + // This default constructor is only used in ArithNonLinIntExpr ctor to allocate memory + // Afterwards, LinIntExpr is initialized for each operation by setting up the a array, + // which would leak memory if a node is allocated here } LinIntExpr::LinIntExpr(int c) : @@ -542,7 +540,7 @@ namespace Gecode { const LinIntExpr& LinIntExpr::operator =(const LinIntExpr& e) { if (this != &e) { - if (n->decrement()) + if (n != nullptr && n->decrement()) delete n; n = e.n; n->use++; } @@ -550,7 +548,7 @@ namespace Gecode { } LinIntExpr::~LinIntExpr(void) { - if (n->decrement()) + if (n !=nullptr && n->decrement()) delete n; } From fee9b3b2c4a0358ddddfac52dc5eb6b7cf39434b Mon Sep 17 00:00:00 2001 From: Kris Coester Date: Thu, 30 Oct 2025 15:00:04 +0100 Subject: [PATCH 14/14] Adapt locking mechanism to be minimal with respect to locked scope (global) --- gecode/kernel/core.cpp | 7 ++----- gecode/kernel/core.hpp | 5 +---- gecode/kernel/gpi.cpp | 2 -- gecode/kernel/gpi.hpp | 23 ++++++++--------------- gecode/kernel/memory/manager.cpp | 3 --- gecode/kernel/memory/manager.hpp | 3 ++- gecode/kernel/trace/tracer.hpp | 16 ++++++++++++++++ gecode/search/tracer.hpp | 14 +++++++++++--- 8 files changed, 40 insertions(+), 33 deletions(-) diff --git a/gecode/kernel/core.cpp b/gecode/kernel/core.cpp index 6158575028..b3ca83c9af 100755 --- a/gecode/kernel/core.cpp +++ b/gecode/kernel/core.cpp @@ -959,14 +959,11 @@ namespace Gecode { BrancherGroup BrancherGroup::all(GROUPID_ALL); BrancherGroup BrancherGroup::def(GROUPID_DEF); - unsigned int Group::next = GROUPID_DEF+1; - Support::Mutex Group::m; - + std::atomic_uint Group::next{GROUPID_DEF+1}; Group::Group(void) { { - Support::Lock l(m); - gid = next++; + gid = next.fetch_add(1); } if (gid == GROUPID_MAX) throw TooManyGroups("Group::Group"); diff --git a/gecode/kernel/core.hpp b/gecode/kernel/core.hpp index 28a841ad7a..1298ec5d0c 100755 --- a/gecode/kernel/core.hpp +++ b/gecode/kernel/core.hpp @@ -692,10 +692,7 @@ namespace Gecode { unsigned int gid; /// Next group id GECODE_KERNEL_EXPORT - static unsigned int next; - /// Mutex for protection - GECODE_KERNEL_EXPORT - static Support::Mutex m; + static std::atomic_uint next; /// Construct with predefined group id \a gid0 Group(unsigned int gid0); public: diff --git a/gecode/kernel/gpi.cpp b/gecode/kernel/gpi.cpp index 8525488d55..4456a1934c 100644 --- a/gecode/kernel/gpi.cpp +++ b/gecode/kernel/gpi.cpp @@ -35,8 +35,6 @@ namespace Gecode { namespace Kernel { - Support::Mutex GPI::m; - }} // STATISTICS: kernel-prop diff --git a/gecode/kernel/gpi.hpp b/gecode/kernel/gpi.hpp index 8c95daa910..63167a43f3 100755 --- a/gecode/kernel/gpi.hpp +++ b/gecode/kernel/gpi.hpp @@ -78,7 +78,7 @@ namespace Gecode { namespace Kernel { /// The first block Block fst; /// Mutex to synchronize globally shared access - GECODE_KERNEL_EXPORT static Support::Mutex m; + Support::Mutex m; public: /// Initialize GPI(void); @@ -124,58 +124,52 @@ namespace Gecode { namespace Kernel { forceinline void GPI::fail(Info& c) { - m.acquire(); + Support::Lock l(m); c.afc = invd * (c.afc + 1.0); if (c.afc > Kernel::Config::rescale_limit) for (Block* i = b; i != NULL; i = i->next) i->rescale(); - m.release(); } forceinline double GPI::decay(void) const { double d; - const_cast(*this).m.acquire(); + Support::Lock l(const_cast(*this).m); d = 1.0 / invd; - const_cast(*this).m.release(); return d; } forceinline unsigned int GPI::pid(void) const { unsigned int p; - const_cast(*this).m.acquire(); + Support::Lock l(const_cast(*this).m); p = npid; - const_cast(*this).m.release(); return p; } forceinline bool GPI::unshare(void) { bool u; - m.acquire(); + Support::Lock l(m); u = us; us = true; - m.release(); return u; } forceinline void GPI::decay(double d) { - m.acquire(); + Support::Lock l(m); invd = 1.0 / d; - m.release(); } forceinline GPI::Info* GPI::allocate(unsigned int p, unsigned int gid) { Info* c; - m.acquire(); + Support::Lock l(m); if (b->free == 0) { Block* n = new Block; n->next = b; b = n; } c = &b->info[--b->free]; - m.release(); c->init(p,gid); return c; } @@ -183,14 +177,13 @@ namespace Gecode { namespace Kernel { forceinline GPI::Info* GPI::allocate(unsigned int gid) { Info* c; - m.acquire(); + Support::Lock l(m); if (b->free == 0) { Block* n = new Block; n->next = b; b = n; } c = &b->info[--b->free]; c->init(npid++,gid); - m.release(); return c; } diff --git a/gecode/kernel/memory/manager.cpp b/gecode/kernel/memory/manager.cpp index 7537770cab..997f3d66c7 100644 --- a/gecode/kernel/memory/manager.cpp +++ b/gecode/kernel/memory/manager.cpp @@ -36,7 +36,6 @@ namespace Gecode { namespace Kernel { Support::Mutex& SharedMemory::m(void) { - static Support::Mutex _m; return _m; } @@ -50,5 +49,3 @@ namespace Gecode { namespace Kernel { }} // STATISTICS: kernel-memory - - diff --git a/gecode/kernel/memory/manager.hpp b/gecode/kernel/memory/manager.hpp index 4465e76d0f..4ad1158b09 100755 --- a/gecode/kernel/memory/manager.hpp +++ b/gecode/kernel/memory/manager.hpp @@ -66,8 +66,9 @@ namespace Gecode { namespace Kernel { /// A list of cached heap chunks HeapChunk* hc; } heap; + Support::Mutex _m; /// A mutex for access - GECODE_KERNEL_EXPORT static Support::Mutex& m(void); + GECODE_KERNEL_EXPORT Support::Mutex& m(void); public: /// Initialize SharedMemory(void); diff --git a/gecode/kernel/trace/tracer.hpp b/gecode/kernel/trace/tracer.hpp index 9bb35aad02..e448f75bd5 100755 --- a/gecode/kernel/trace/tracer.hpp +++ b/gecode/kernel/trace/tracer.hpp @@ -256,7 +256,9 @@ namespace Gecode { forceinline void ViewTracer::_init(const Space& home, const ViewTraceRecorder& t) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif init(home,t); } template @@ -265,28 +267,36 @@ namespace Gecode { const ViewTraceRecorder& t, const ViewTraceInfo& vti, int i, typename TraceTraits::TraceDelta& d) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif prune(home,t,vti,i,d); } template forceinline void ViewTracer::_fail(const Space& home, const ViewTraceRecorder& t) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif fail(home,t); } template forceinline void ViewTracer::_fix(const Space& home, const ViewTraceRecorder& t) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif fix(home,t); } template forceinline void ViewTracer::_done(const Space& home, const ViewTraceRecorder& t) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif done(home,t); } @@ -307,19 +317,25 @@ namespace Gecode { forceinline void Tracer::_propagate(const Space& home, const PropagateTraceInfo& pti) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif propagate(home,pti); } forceinline void Tracer::_commit(const Space& home, const CommitTraceInfo& cti) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif commit(home,cti); } forceinline void Tracer::_post(const Space& home, const PostTraceInfo& pti) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif post(home,pti); } diff --git a/gecode/search/tracer.hpp b/gecode/search/tracer.hpp index 13349b7bc4..b1b5d278e9 100755 --- a/gecode/search/tracer.hpp +++ b/gecode/search/tracer.hpp @@ -81,7 +81,7 @@ namespace Gecode { assert((type() == EngineType::RBS) || (type() == EngineType::PBS)); return _fst; } - + forceinline unsigned int SearchTracer::EngineInfo::elst(void) const { assert((type() == EngineType::RBS) || (type() == EngineType::PBS)); @@ -91,7 +91,7 @@ namespace Gecode { forceinline unsigned int SearchTracer::EngineInfo::engines(void) const { return elst() - efst(); - } + } /* @@ -200,24 +200,30 @@ namespace Gecode { */ forceinline void SearchTracer::_round(unsigned int eid) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif round(eid); } forceinline void SearchTracer::_skip(const EdgeInfo& ei) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif skip(ei); } forceinline void SearchTracer::_node(const EdgeInfo& ei, const NodeInfo& ni) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif node(ei,ni); } forceinline - SearchTracer::SearchTracer(void) + SearchTracer::SearchTracer(void) : pending(1U), n_e(0U), n_w(0U), es(heap), w2e(heap) {} forceinline void @@ -252,7 +258,9 @@ namespace Gecode { forceinline void SearchTracer::worker(void) { +#ifdef GECODE_HAS_SYNCHED_TRACE Support::Lock l(m); +#endif if (--n_active == 0U) done(); }