Skip to content

Commit 3630de1

Browse files
fix: harden lifting name allocation and signature arity guard
Prevent variable name collisions when lifting creates virtual vars by checking existing names before allocating. Extract CompactSignature dispatch helper in AuxVarEliminator to deduplicate platform-specific compaction logic. Add arity guard in RunPrepareCoeffModel to block coefficient interpolation on malformed signatures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent ba9954a commit 3630de1

4 files changed

Lines changed: 122 additions & 22 deletions

File tree

lib/core/AuxVarEliminator.cpp

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,19 @@ namespace cobra {
166166

167167
#endif // COBRA_SVE2
168168

169+
std::vector< uint64_t > CompactSignature(
170+
const std::vector< uint64_t > &sig, uint64_t live_mask, uint32_t num_vars
171+
) {
172+
#if COBRA_X86
173+
if (has_bmi2()) { return CompactSignatureHw(sig, live_mask, num_vars); }
174+
return CompactSignatureSoft(sig, live_mask, num_vars);
175+
#elif COBRA_SVE2
176+
return CompactSignatureSve2(sig, live_mask, num_vars);
177+
#else
178+
return CompactSignatureSoft(sig, live_mask, num_vars);
179+
#endif
180+
}
181+
169182
} // namespace
170183

171184
EliminationResult EliminateAuxVars(
@@ -182,22 +195,8 @@ namespace cobra {
182195

183196
const uint64_t kLiveMask = DetectLiveMask(sig, kNumVars);
184197

185-
// Compact the signature vector in a single pass
186-
std::vector< uint64_t > reduced;
187-
#if COBRA_X86
188-
if (has_bmi2()) {
189-
reduced = CompactSignatureHw(sig, kLiveMask, kNumVars);
190-
} else {
191-
reduced = CompactSignatureSoft(sig, kLiveMask, kNumVars);
192-
}
193-
#elif COBRA_SVE2
194-
reduced = CompactSignatureSve2(sig, kLiveMask, kNumVars);
195-
#else
196-
reduced = CompactSignatureSoft(sig, kLiveMask, kNumVars);
197-
#endif
198-
199198
EliminationResult result;
200-
result.reduced_sig = std::move(reduced);
199+
result.reduced_sig = CompactSignature(sig, kLiveMask, kNumVars);
201200
for (uint32_t v = 0; v < kNumVars; ++v) {
202201
if ((kLiveMask & (1ULL << v)) != 0u) {
203202
result.real_vars.push_back(vars[v]);
@@ -252,7 +251,7 @@ namespace cobra {
252251
std::sort(result.real_vars.begin(), result.real_vars.end(), by_original_index);
253252
std::sort(result.spurious_vars.begin(), result.spurious_vars.end(), by_original_index);
254253

255-
result.reduced_sig = CompactSignatureSoft(sig, live_mask, kNumVars);
254+
result.reduced_sig = CompactSignature(sig, live_mask, kNumVars);
256255

257256
return result;
258257
}

lib/core/LiftingPasses.cpp

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <algorithm>
1010
#include <functional>
1111
#include <unordered_map>
12+
#include <unordered_set>
1213

1314
namespace cobra {
1415

@@ -120,6 +121,24 @@ namespace cobra {
120121
uint32_t virtual_index;
121122
};
122123

124+
std::vector< std::string > AllocateFreshVirtualNames(
125+
const std::vector< std::string > &existing_vars, const std::string &prefix,
126+
size_t count
127+
) {
128+
std::unordered_set< std::string > used(existing_vars.begin(), existing_vars.end());
129+
std::vector< std::string > names;
130+
names.reserve(count);
131+
132+
size_t next_suffix = 0;
133+
while (names.size() < count) {
134+
auto candidate = prefix + std::to_string(next_suffix++);
135+
if (!used.insert(candidate).second) { continue; }
136+
names.push_back(std::move(candidate));
137+
}
138+
139+
return names;
140+
}
141+
123142
std::vector< DeduplicatedAtom > DeduplicateAtoms(
124143
const std::vector< LiftCandidate > &candidates, uint32_t first_virtual_index
125144
) {
@@ -363,9 +382,11 @@ namespace cobra {
363382

364383
// Build extended variable list: original vars + virtual vars.
365384
std::vector< std::string > outer_vars = vars;
366-
for (size_t i = 0; i < atoms.size(); ++i) {
367-
outer_vars.push_back("v" + std::to_string(i));
368-
}
385+
auto virtual_names = AllocateFreshVirtualNames(vars, "v", atoms.size());
386+
outer_vars.insert(
387+
outer_vars.end(), std::make_move_iterator(virtual_names.begin()),
388+
std::make_move_iterator(virtual_names.end())
389+
);
369390

370391
// Build bindings.
371392
std::vector< LiftedBinding > bindings;
@@ -524,9 +545,11 @@ namespace cobra {
524545

525546
// Build extended variable list: original vars + virtual vars.
526547
std::vector< std::string > outer_vars = vars;
527-
for (size_t i = 0; i < selected.size(); ++i) {
528-
outer_vars.push_back("r" + std::to_string(i));
529-
}
548+
auto virtual_names = AllocateFreshVirtualNames(vars, "r", selected.size());
549+
outer_vars.insert(
550+
outer_vars.end(), std::make_move_iterator(virtual_names.begin()),
551+
std::make_move_iterator(virtual_names.end())
552+
);
530553

531554
// Build bindings — use atoms (which have copies of
532555
// rendered strings) instead of selected for subtree access.

lib/core/SignaturePasses.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,6 +1054,27 @@ namespace cobra {
10541054
const auto &sub_ctx = sig_payload.ctx;
10551055
const auto &sig = sub_ctx.elimination.reduced_sig;
10561056
const auto num_vars = static_cast< uint32_t >(sub_ctx.real_vars.size());
1057+
const auto expected_len = size_t{ 1 } << num_vars;
1058+
1059+
if (sig.size() != expected_len) {
1060+
return Ok(
1061+
PassResult{
1062+
.decision = PassDecision::kBlocked,
1063+
.disposition = ItemDisposition::kRetainCurrent,
1064+
.reason =
1065+
ReasonDetail{
1066+
.top = {
1067+
.code = {
1068+
ReasonCategory::kGuardFailed,
1069+
ReasonDomain::kSignature,
1070+
},
1071+
.message =
1072+
"Reduced signature arity does not match the active variable set",
1073+
},
1074+
},
1075+
}
1076+
);
1077+
}
10571078

10581079
auto coeffs = InterpolateCoefficients(sig, num_vars, ctx.bitwidth);
10591080

test/core/test_lifting_passes.cpp

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,34 @@ TEST(ArithmeticAtomLifter, CarriesParentLocalSolveContext) {
9696
EXPECT_EQ((*skel->original_ctx.evaluator)(std::vector< uint64_t >{ 3, 5 }), 1u);
9797
}
9898

99+
TEST(ArithmeticAtomLifter, UsesFreshVirtualNamesAgainstExistingLocals) {
100+
auto expr =
101+
Expr::BitwiseAnd(Expr::Mul(Expr::Variable(0), Expr::Variable(0)), Expr::Variable(1));
102+
auto cls = ClassifyStructural(*expr);
103+
104+
Options opts{ .bitwidth = 64, .max_vars = 16 };
105+
auto ctx = MakeLiftCtx(opts, { "x" });
106+
107+
WorkItem item;
108+
item.payload = AstPayload{
109+
.expr = std::move(expr),
110+
.classification = cls,
111+
.provenance = Provenance::kRewritten,
112+
.solve_ctx = AstSolveContext{ .vars = { "x", "v0" } },
113+
};
114+
item.features.classification = cls;
115+
item.features.provenance = Provenance::kRewritten;
116+
117+
auto result = RunLiftArithmeticAtoms(item, ctx);
118+
ASSERT_TRUE(result.has_value());
119+
auto &pr = result.value();
120+
ASSERT_EQ(pr.decision, PassDecision::kAdvance);
121+
122+
auto *skel = std::get_if< LiftedSkeletonPayload >(&pr.next[0].payload);
123+
ASSERT_NE(skel, nullptr);
124+
EXPECT_EQ(skel->outer_ctx.vars, (std::vector< std::string >{ "x", "v0", "v1" }));
125+
}
126+
99127
// x & y -- no arithmetic atoms
100128
TEST(ArithmeticAtomLifter, NoAtomsReturnsNotApplicable) {
101129
auto expr = Expr::BitwiseAnd(Expr::Variable(0), Expr::Variable(1));
@@ -192,6 +220,35 @@ TEST(RepeatedSubexprLifter, LiftsLargeRepeatedSubtrees) {
192220
EXPECT_EQ(skel->bindings[0].kind, LiftedValueKind::kRepeatedSubexpression);
193221
}
194222

223+
TEST(RepeatedSubexprLifter, UsesFreshVirtualNamesAgainstExistingLocals) {
224+
auto repeated = []() {
225+
return Expr::Mul(
226+
Expr::Add(Expr::Variable(0), Expr::Variable(2)),
227+
Expr::Add(Expr::Variable(0), Expr::Variable(2))
228+
);
229+
};
230+
auto expr = Expr::BitwiseXor(repeated(), repeated());
231+
Options opts{ .bitwidth = 64, .max_vars = 16 };
232+
auto ctx = MakeLiftCtx(opts, { "b" });
233+
234+
WorkItem item;
235+
item.payload = AstPayload{
236+
.expr = std::move(expr),
237+
.provenance = Provenance::kRewritten,
238+
.solve_ctx = AstSolveContext{ .vars = { "b", "r1", "r0" } },
239+
};
240+
item.features.provenance = Provenance::kRewritten;
241+
242+
auto result = RunLiftRepeatedSubexpressions(item, ctx);
243+
ASSERT_TRUE(result.has_value());
244+
auto &pr = result.value();
245+
ASSERT_EQ(pr.decision, PassDecision::kAdvance);
246+
247+
auto *skel = std::get_if< LiftedSkeletonPayload >(&pr.next[0].payload);
248+
ASSERT_NE(skel, nullptr);
249+
EXPECT_EQ(skel->outer_ctx.vars, (std::vector< std::string >{ "b", "r1", "r0", "r2" }));
250+
}
251+
195252
TEST(RepeatedSubexprLifter, MaximalNonOverlapping) {
196253
// outer = Mul(Add(x,y), Add(x,y)) — appears 2x
197254
// inner = Add(x,y) — appears 4x but size < 4

0 commit comments

Comments
 (0)