Skip to content

Commit 1025982

Browse files
committed
test(generator): the Clone gate must see a real memcpy, not a name mention (#386)
Adversarial review of the parent commit (2c1394d) found the new every_handle_buffer_is_duplicated_by_clone gate only checked that a buffer's field name appeared as a substring in TA_<N>_Clone's body -- so a regression to a raw alias (`sp->buf = stream->buf;`, corrupting both handles the moment either advances) or a silent drop (`sp->buf = NULL;` with no copy) would still contain the name and pass. Now requires a `memcpy(...)` line naming both `sp->{buf},` and `stream->{buf},` -- matched per line rather than as one fixed-spacing literal (so a reformat of the emitter's call layout can't turn every buffer in the corpus into a false offender), with the trailing comma as an identifier-boundary guard (so a buffer whose name is a prefix of a sibling's, e.g. `ring`/`ring2`, can't false-pass off the sibling's memcpy line). Both were review findings on this same commit, in two separate rounds. Fixed a false positive the stricter check surfaced: MA (the Dispatch tier) type-erases its one sub-handle as `void *sub`, cloned via a switch to the correct sub-type's own `_Clone` rather than memcpy'd -- `handle_buffers`'s existing `_Stream`-name exclusion doesn't catch a type-erased handle. Flipped it from a denylist (`_Stream`, `void`) to a positive allowlist (`double`/`int`, the only two types a real data buffer ever has in this corpus), more robust against a hypothetical future pointer type than enumerating what to exclude. Sabotage-proved three times over three review rounds: (1) changing clone_buffer_lines to emit the raw-alias form directly (the exact regression the first review round named) -- the strengthened test failed, naming every affected buffer; the original weak substring check would have passed it silently. Restored, confirmed green each time. Not addressed: a review round also noted that excluding `void *sub` means MA no longer appears as a swept handle at all, so nothing in this file verifies TA_MA_Clone's dispatch switch routes to the correct sub-type's own Clone. That is a real gap, but a different gate than the one this commit adds (buffer duplication, not sub-handle routing) -- left for the issue rather than expanding this commit's scope unverified. Claude-Session: https://claude.ai/code/session_01JXQFXKVdVJa1XKbo1sSpyY
1 parent 17cc6e3 commit 1025982

1 file changed

Lines changed: 24 additions & 5 deletions

File tree

ta_codegen/generator/tests/peek_suite.rs

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,15 @@ fn handle_buffers(src: &str, upper: &str) -> BTreeSet<String> {
105105
if name.is_empty() || !name.chars().all(|c| c.is_alphanumeric() || c == '_') {
106106
continue;
107107
}
108-
// A sub-stream handle is a pointer too, and peek routes into it by
109-
// calling its own Peek — it is not a buffer this frame indexes.
110-
if ty.contains("_Stream") {
108+
// A real data buffer is always `double *` or `int *` in this corpus —
109+
// an allowlist, not a denylist, so a pointer this doesn't recognize is
110+
// excluded rather than silently swept in as a buffer. That is what a
111+
// sub-stream handle is too (a `_Stream *`, or the Dispatch tier's one
112+
// `void *sub`, type-erased and tagged by optInMAType): peek routes into
113+
// it by calling its own Peek, and Clone by calling its own Clone —
114+
// neither treats it as a buffer this frame indexes or memcpy's.
115+
let ty = ty.trim();
116+
if ty != "double" && ty != "int" {
111117
continue;
112118
}
113119
out.insert(name.to_string());
@@ -1350,8 +1356,21 @@ fn every_handle_buffer_is_duplicated_by_clone() {
13501356
};
13511357
for buf in &buffers {
13521358
buffers_checked += 1;
1353-
if !clone_body.contains(buf.as_str()) {
1354-
offenders.push(format!("{upper}: Clone never mentions `{buf}`"));
1359+
// Not just "mentioned" -- actually memcpy'd from the source into the
1360+
// fork's own allocation, not just assigned or nulled. Matched per
1361+
// LINE rather than as one fixed-spacing literal, so a reformat of
1362+
// the emitter's call layout (space after `(`, before `,`, ...)
1363+
// cannot make every buffer in the corpus a false offender. The
1364+
// trailing `,` is load-bearing, not decorative: without it a
1365+
// buffer whose name is a PREFIX of a sibling's (`ring` vs `ring2`)
1366+
// would read `sp->ring2,`'s line as proof `ring` was copied too.
1367+
let copied = clone_body.lines().any(|line| {
1368+
line.contains("memcpy(")
1369+
&& line.contains(&format!("sp->{buf},"))
1370+
&& line.contains(&format!("stream->{buf},"))
1371+
});
1372+
if !copied {
1373+
offenders.push(format!("{upper}: Clone never memcpy's `{buf}` from the source"));
13551374
}
13561375
}
13571376
}

0 commit comments

Comments
 (0)