From 009664d306206356e6ef4b549ef6567d15aead00 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Tue, 7 Jan 2025 11:53:52 +0100 Subject: [PATCH] internal/core/adt: fix closedness issue related to sharing A closedness issue could arise when a vertex was unified multiple times within the same node if this vertex was (initially) shared. Basically, the when a shared node was added again, it was dropped based on pointer equivalence. If then later this node was unshared, this node was no longer added, meaning that arcs could be missing from closedness information, causing arcs to be incorrectly defined as missing. We now ensure the node is added multiple times, just like when structure sharing is off, to ensure closedness data is correctly computed. Note that this also fixes issue 3527. This was not marked as problematic, as the original CL just fixed the panic. But the result was still incorrect. Fixes #3641 Fixes #3546 Issue #3527 Signed-off-by: Marcel van Lohuizen Change-Id: I9c248b0448ae1bcfcf4e4c1832397d73e8b02705 Dispatch-Trailer: {"type":"trybot","CL":1206823,"patchset":1,"ref":"refs/changes/23/1206823/1","targetBranch":"master"} --- cue/testdata/cycle/issue3527.txtar | 87 +++------ cue/testdata/eval/sharing.txtar | 286 +++++------------------------ internal/core/adt/conjunct.go | 3 + internal/core/adt/eval.go | 4 + internal/core/adt/share.go | 44 +++-- 5 files changed, 102 insertions(+), 322 deletions(-) diff --git a/cue/testdata/cycle/issue3527.txtar b/cue/testdata/cycle/issue3527.txtar index 4b9e37195..0a03f149f 100644 --- a/cue/testdata/cycle/issue3527.txtar +++ b/cue/testdata/cycle/issue3527.txtar @@ -30,14 +30,7 @@ Unifications: 35 Conjuncts: 82 Disjuncts: 39 -- out/evalalpha -- -Errors: -assert.res._computed.in.taxPayer.self: field not allowed: - ./in.cue:5:22 - ./in.cue:18:12 - -Result: -(_|_){ - // [eval] +(struct){ _taxPayer(:p): (struct){ self: (struct){ } @@ -45,29 +38,21 @@ Result: let sam#1 = (struct){ taxPayer: ~(_taxPayer(:p)) } - assert: (_|_){ - // [eval] + assert: (struct){ invoke: (struct){ taxPayer: ~(_taxPayer(:p)) } - res: (_|_){ - // [eval] + res: (struct){ in: (#struct){ taxPayer: (#struct){ self: (#struct){ } } } - _computed(:p): (_|_){ - // [eval] - in: (_|_){ - // [eval] - taxPayer: (_|_){ - // [eval] - self: (_|_){ - // [eval] assert.res._computed.in.taxPayer.self: field not allowed: - // ./in.cue:5:22 - // ./in.cue:18:12 + _computed(:p): (#struct){ + in: (#struct){ + taxPayer: (#struct){ + self: (#struct){ } } } @@ -94,18 +79,7 @@ Result: diff old new --- old +++ new -@@ -1,32 +1,41 @@ --(struct){ -+Errors: -+assert.res._computed.in.taxPayer.self: field not allowed: -+ ./in.cue:5:22 -+ ./in.cue:18:12 -+ -+Result: -+(_|_){ -+ // [eval] - _taxPayer(:p): (struct){ - self: (struct){ +@@ -4,22 +4,16 @@ } } let sam#1 = (struct){ @@ -113,51 +87,34 @@ diff old new - self: (struct){ - } - } -- } -- assert: (struct){ + taxPayer: ~(_taxPayer(:p)) -+ } -+ assert: (_|_){ -+ // [eval] + } + assert: (struct){ invoke: (struct){ - taxPayer: (struct){ - self: (struct){ - } - } -- } -- res: (struct){ + taxPayer: ~(_taxPayer(:p)) -+ } -+ res: (_|_){ -+ // [eval] + } + res: (struct){ in: (#struct){ taxPayer: (#struct){ - self: (struct){ -- } -- } -- } -- _computed(:p): (#struct){ -- in: (#struct){ -- taxPayer: (#struct){ -- self: (struct){ + self: (#struct){ -+ } -+ } -+ } -+ _computed(:p): (_|_){ -+ // [eval] -+ in: (_|_){ -+ // [eval] -+ taxPayer: (_|_){ -+ // [eval] -+ self: (_|_){ -+ // [eval] assert.res._computed.in.taxPayer.self: field not allowed: -+ // ./in.cue:5:22 -+ // ./in.cue:18:12 + } + } + } +@@ -26,7 +20,7 @@ + _computed(:p): (#struct){ + in: (#struct){ + taxPayer: (#struct){ +- self: (struct){ ++ self: (#struct){ } } } -@@ -34,30 +43,16 @@ +@@ -34,30 +28,16 @@ } } subject: (struct){ diff --git a/cue/testdata/eval/sharing.txtar b/cue/testdata/eval/sharing.txtar index 3ee1dfd2e..a63af1e6a 100644 --- a/cue/testdata/eval/sharing.txtar +++ b/cue/testdata/eval/sharing.txtar @@ -132,31 +132,10 @@ Unifications: 150 Conjuncts: 309 Disjuncts: 165 -- out/evalalpha -- -Errors: -issue3641.simplified.t1.out.cfg.ctx: field not allowed: - ./dupshare.cue:13:16 - ./dupshare.cue:6:13 -issue3641.simplified.t2.out.cfg.ctx: field not allowed: - ./dupshare.cue:29:16 - ./dupshare.cue:22:13 -issue3641.simplified.t3.out.cfg.ctx: field not allowed: - ./dupshare.cue:46:16 - ./dupshare.cue:39:13 -issue3641.full.out.sch.cfg.ctx: field not allowed: - ./dupshare.cue:60:17 - ./dupshare.cue:56:13 -issue3546.reduced.out.list.0: field not allowed: - ./dupshare.cue:72:7 - -Result: -(_|_){ - // [eval] - issue3641: (_|_){ - // [eval] - simplified: (_|_){ - // [eval] - t1: (_|_){ - // [eval] +(struct){ + issue3641: (struct){ + simplified: (struct){ + t1: (struct){ #Context1: (#struct){ ctx: (#struct){ } @@ -180,20 +159,14 @@ Result: } } } - out: (_|_){ - // [eval] - cfg: (_|_){ - // [eval] - ctx: (_|_){ - // [eval] issue3641.simplified.t1.out.cfg.ctx: field not allowed: - // ./dupshare.cue:13:16 - // ./dupshare.cue:6:13 + out: (#struct){ + cfg: (#struct){ + ctx: (#struct){ } } } } - t2: (_|_){ - // [eval] + t2: (struct){ #Context1: (#struct){ ctx: (#struct){ } @@ -217,20 +190,14 @@ Result: } } } - out: (_|_){ - // [eval] - cfg: (_|_){ - // [eval] - ctx: (_|_){ - // [eval] issue3641.simplified.t2.out.cfg.ctx: field not allowed: - // ./dupshare.cue:29:16 - // ./dupshare.cue:22:13 + out: (#struct){ + cfg: (#struct){ + ctx: (#struct){ } } } } - t3: (_|_){ - // [eval] + t3: (struct){ #Context1: (#struct){ ctx: (#struct){ } @@ -254,21 +221,15 @@ Result: } } } - out: (_|_){ - // [eval] - cfg: (_|_){ - // [eval] - ctx: (_|_){ - // [eval] issue3641.simplified.t3.out.cfg.ctx: field not allowed: - // ./dupshare.cue:46:16 - // ./dupshare.cue:39:13 + out: (#struct){ + cfg: (#struct){ + ctx: (#struct){ } } } } } - full: (_|_){ - // [eval] + full: (struct){ #Context1: (#struct){ ctx: (#struct){ } @@ -293,26 +254,18 @@ Result: } } let config#1 = ~(issue3641.full.#Config) - out: (_|_){ - // [eval] - sch: (_|_){ - // [eval] - cfg: (_|_){ - // [eval] - ctx: (_|_){ - // [eval] issue3641.full.out.sch.cfg.ctx: field not allowed: - // ./dupshare.cue:60:17 - // ./dupshare.cue:56:13 + out: (#struct){ + sch: (#struct){ + cfg: (#struct){ + ctx: (#struct){ } } } } } } - issue3546: (_|_){ - // [eval] - reduced: (_|_){ - // [eval] + issue3546: (struct){ + reduced: (struct){ all: (#list){ 0: (string){ "a" } } @@ -330,14 +283,9 @@ Result: 0: (string){ "a" } } } - out: (_|_){ - // [eval] - list: (_|_){ - // [eval] - 0: (_|_){ - // [eval] issue3546.reduced.out.list.0: field not allowed: - // ./dupshare.cue:72:7 - } + out: (#struct){ + list: (#list){ + 0: (string){ "a" } } } } @@ -411,20 +359,10 @@ Result: diff old new --- old +++ new -@@ -1,128 +1,140 @@ --(struct){ -- issue3641: (struct){ -- simplified: (struct){ -- t1: (struct){ -- #Context1: (#struct){ -- ctx: (#struct){ -- } -- } -- Context2: (struct){ -- ctx: (struct){ -- } -- } -- #Config1: (#struct){ +@@ -11,102 +11,75 @@ + } + } + #Config1: (#struct){ - cfg: (#struct){ - ctx: (#struct){ - } @@ -521,56 +459,6 @@ diff old new - ctx: (struct){ - } - } -- } -- Config: (#struct){ -- cfg: (#struct){ -- ctx: (#struct){ -- } -- } -- } -- out: (#struct){ -- cfg: (#struct){ -- ctx: (#struct){ -- } -- } -- } -- } -- } -- full: (struct){ -+Errors: -+issue3641.simplified.t1.out.cfg.ctx: field not allowed: -+ ./dupshare.cue:13:16 -+ ./dupshare.cue:6:13 -+issue3641.simplified.t2.out.cfg.ctx: field not allowed: -+ ./dupshare.cue:29:16 -+ ./dupshare.cue:22:13 -+issue3641.simplified.t3.out.cfg.ctx: field not allowed: -+ ./dupshare.cue:46:16 -+ ./dupshare.cue:39:13 -+issue3641.full.out.sch.cfg.ctx: field not allowed: -+ ./dupshare.cue:60:17 -+ ./dupshare.cue:56:13 -+issue3546.reduced.out.list.0: field not allowed: -+ ./dupshare.cue:72:7 -+ -+Result: -+(_|_){ -+ // [eval] -+ issue3641: (_|_){ -+ // [eval] -+ simplified: (_|_){ -+ // [eval] -+ t1: (_|_){ -+ // [eval] -+ #Context1: (#struct){ -+ ctx: (#struct){ -+ } -+ } -+ Context2: (struct){ -+ ctx: (struct){ -+ } -+ } -+ #Config1: (#struct){ + cfg: ~(issue3641.simplified.t1.#Context1) + } + #Config3: (#struct){ @@ -585,20 +473,14 @@ diff old new + } + } + } -+ out: (_|_){ -+ // [eval] -+ cfg: (_|_){ -+ // [eval] -+ ctx: (_|_){ -+ // [eval] issue3641.simplified.t1.out.cfg.ctx: field not allowed: -+ // ./dupshare.cue:13:16 -+ // ./dupshare.cue:6:13 ++ out: (#struct){ ++ cfg: (#struct){ ++ ctx: (#struct){ + } + } + } + } -+ t2: (_|_){ -+ // [eval] ++ t2: (struct){ + #Context1: (#struct){ + ctx: (#struct){ + } @@ -622,20 +504,14 @@ diff old new + } + } + } -+ out: (_|_){ -+ // [eval] -+ cfg: (_|_){ -+ // [eval] -+ ctx: (_|_){ -+ // [eval] issue3641.simplified.t2.out.cfg.ctx: field not allowed: -+ // ./dupshare.cue:29:16 -+ // ./dupshare.cue:22:13 ++ out: (#struct){ ++ cfg: (#struct){ ++ ctx: (#struct){ + } + } + } + } -+ t3: (_|_){ -+ // [eval] ++ t3: (struct){ + #Context1: (#struct){ + ctx: (#struct){ + } @@ -652,32 +528,10 @@ diff old new + } + Config2: (struct){ + cfg: ~(issue3641.simplified.t3.Context2) -+ } -+ Config: (#struct){ -+ cfg: (#struct){ -+ ctx: (#struct){ -+ } -+ } -+ } -+ out: (_|_){ -+ // [eval] -+ cfg: (_|_){ -+ // [eval] -+ ctx: (_|_){ -+ // [eval] issue3641.simplified.t3.out.cfg.ctx: field not allowed: -+ // ./dupshare.cue:46:16 -+ // ./dupshare.cue:39:13 -+ } -+ } -+ } -+ } -+ } -+ full: (_|_){ -+ // [eval] - #Context1: (#struct){ - ctx: (#struct){ } -@@ -132,24 +144,13 @@ + Config: (#struct){ + cfg: (#struct){ +@@ -132,24 +105,13 @@ } } #Config1: (#struct){ @@ -705,7 +559,7 @@ diff old new } #Config: (#struct){ cfg: (#struct){ -@@ -157,24 +158,27 @@ +@@ -157,12 +119,7 @@ } } } @@ -715,61 +569,11 @@ diff old new - } - } - } -- out: (#struct){ -- sch: (#struct){ -- cfg: (#struct){ -- ctx: (#struct){ -- } -- } -- } -- } -- } -- } -- issue3546: (struct){ -- reduced: (struct){ + let config#1 = ~(issue3641.full.#Config) -+ out: (_|_){ -+ // [eval] -+ sch: (_|_){ -+ // [eval] -+ cfg: (_|_){ -+ // [eval] -+ ctx: (_|_){ -+ // [eval] issue3641.full.out.sch.cfg.ctx: field not allowed: -+ // ./dupshare.cue:60:17 -+ // ./dupshare.cue:56:13 -+ } -+ } -+ } -+ } -+ } -+ } -+ issue3546: (_|_){ -+ // [eval] -+ reduced: (_|_){ -+ // [eval] - all: (#list){ - 0: (string){ "a" } - } -@@ -192,9 +196,14 @@ - 0: (string){ "a" } - } - } -- out: (#struct){ -- list: (#list){ -- 0: (string){ "a" } -+ out: (_|_){ -+ // [eval] -+ list: (_|_){ -+ // [eval] -+ 0: (_|_){ -+ // [eval] issue3546.reduced.out.list.0: field not allowed: -+ // ./dupshare.cue:72:7 -+ } - } - } - } -@@ -231,9 +240,7 @@ + out: (#struct){ + sch: (#struct){ + cfg: (#struct){ +@@ -231,9 +188,7 @@ } debug: (struct){ sharingOn: (struct){ diff --git a/internal/core/adt/conjunct.go b/internal/core/adt/conjunct.go index 3cbea4812..6eaf436d5 100644 --- a/internal/core/adt/conjunct.go +++ b/internal/core/adt/conjunct.go @@ -384,6 +384,9 @@ loop2: func (n *nodeContext) scheduleVertexConjuncts(c Conjunct, arc *Vertex, closeInfo CloseInfo) { // disjunctions, we need to dereference he underlying node. if deref(n.node) == deref(arc) { + if n.isShared { + n.addShared(closeInfo) + } return } diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go index a179c58c8..d16278bc7 100644 --- a/internal/core/adt/eval.go +++ b/internal/core/adt/eval.go @@ -1039,6 +1039,8 @@ type nodeContext struct { // TODO: also use this to communicate increasingly more concrete values. notify []receiver + sharedIDs []CloseInfo + // Conjuncts holds a reference to the Vertex Arcs that still need // processing. It does NOT need to be copied. conjuncts []conjunct @@ -1227,6 +1229,7 @@ func (n *nodeContext) clone() *nodeContext { d.arcMap = append(d.arcMap, n.arcMap...) d.notify = append(d.notify, n.notify...) + d.sharedIDs = append(d.sharedIDs, n.sharedIDs...) n.scheduler.cloneInto(&d.scheduler) @@ -1264,6 +1267,7 @@ func (c *OpContext) newNodeContext(node *Vertex) *nodeContext { conjuncts: n.conjuncts[:0], cyclicConjuncts: n.cyclicConjuncts[:0], notify: n.notify[:0], + sharedIDs: n.sharedIDs[:0], checks: n.checks[:0], postChecks: n.postChecks[:0], dynamicFields: n.dynamicFields[:0], diff --git a/internal/core/adt/share.go b/internal/core/adt/share.go index 2ad0a2268..7ffe46a5f 100644 --- a/internal/core/adt/share.go +++ b/internal/core/adt/share.go @@ -45,19 +45,17 @@ func (n *nodeContext) unshare() { // Find another mechanism once we get rid of the old evaluator. n.node.BaseValue = n.origBaseValue - n.scheduleVertexConjuncts(n.shared, v, n.sharedID) + for _, id := range n.sharedIDs { + n.scheduleVertexConjuncts(n.shared, v, id) + } - n.sharedID.cc.decDependent(n.ctx, SHARED, n.node.cc()) - n.sharedID.cc = nil + n.decSharedIDs() } // finalizeSharing should be called when it is known for sure a node can be // shared. func (n *nodeContext) finalizeSharing() { - if n.sharedID.cc != nil { - n.sharedID.cc.decDependent(n.ctx, SHARED, n.node.cc()) - n.sharedID.cc = nil - } + n.decSharedIDs() if !n.isShared { return } @@ -91,6 +89,28 @@ func (n *nodeContext) finalizeSharing() { } } +func (n *nodeContext) addShared(id CloseInfo) { + // At this point, the node may still be unshared at a later point. For this + // purpose we need to keep the retain count above zero until all conjuncts + // have been processed and it is clear that sharing is possible. Delaying + // such a count should not hurt performance, as a shared node is completed + // anyway. + n.sharedIDs = append(n.sharedIDs, id) + if id.cc != nil { + id.cc.incDependent(n.ctx, SHARED, n.node.cc()) + } +} + +func (n *nodeContext) decSharedIDs() { + for _, id := range n.sharedIDs { + if cc := id.cc; cc != nil { + n.Logf("DECSHAREDCCS: %p", cc) + cc.decDependent(n.ctx, SHARED, n.node.cc()) + } + } + n.sharedIDs = n.sharedIDs[:0] +} + func (n *nodeContext) share(c Conjunct, arc *Vertex, id CloseInfo) { if n.isShared { panic("already sharing") @@ -101,6 +121,7 @@ func (n *nodeContext) share(c Conjunct, arc *Vertex, id CloseInfo) { n.isShared = true n.shared = c n.sharedID = id + n.addShared(id) if arc.IsDetached() && arc.MayAttach() { // TODO: Second check necessary? // This node can safely be shared. Since it is not rooted, though, it @@ -114,15 +135,6 @@ func (n *nodeContext) share(c Conjunct, arc *Vertex, id CloseInfo) { s.parent = n.node } } - - // At this point, the node may still be unshared at a later point. For this - // purpose we need to keep the retain count above zero until all conjuncts - // have been processed and it is clear that sharing is possible. Delaying - // such a count should not hurt performance, as a shared node is completed - // anyway. - if id.cc != nil { - id.cc.incDependent(n.ctx, SHARED, n.node.cc()) - } } func (n *nodeContext) shareIfPossible(c Conjunct, arc *Vertex, id CloseInfo) bool {