Two patches, one afternoon

dbglab's random test fails on seed 10, and the hunt in the debug loop has already localised it: the DMA's beat counter left_q is 8 bits wide, a run of 308 words loads 52, and the copy stops early. Two engineers send a patch the same afternoon.

Patch A sets CW = 9: "308 needs nine bits, and the random test never asks for more than 320." Patch B sets CW = $bits(cfg_len). Both make seed 10 pass. Both give the same clean 60-seed sweep. The reviewer has to pick one. Which, and how would you show it rather than argue it?

What people usually do, and why it fails

The usual test of a fix is the test that failed, then the regression. If both are green, the fix is merged. That works when the fix and the cause are the same thing. It fails when a fix covers the cases the tests make and no more, because the regression is exactly those cases. A fix sized to the failure will always look perfect to the tests that found the failure.

The other habit is to argue taste: "nine bits is enough", "deriving the width is cleaner". Arguments do not settle this. A run does, if you choose the input where the two patches disagree.

The method: fix the decision, then test where the fixes differ

1. Name the cause as a decision, not a line. Here it is not "line 36 says 8". It is commit 03ebdc1 declaring the counter's width on its own, as a number, separate from the 10-bit length it must hold. Two widths that must agree were written in two places.

2. Place every candidate fix on the chain from cause to fault to symptom (symptom, fault, cause). A change after the fault, in the checker, the test or the driver, is a symptom fix. A change at the fault that covers only the values you saw is an instance fix: right place, wrong size.

3. Ask what range the cause covers. The register map says LEN is 0 to 1023. The failing test covered at most 320. A fix must hold for the first range, not the second.

4. Find the input where the fixes disagree, and run it on both before choosing. If no such input exists, they are the same fix; if one does, the choice is made by a result.

5. Prefer the fix that makes the bug impossible to write again. A width derived from its source cannot drift. Then remove anything that hides the next mistake, here the cast.

The picture: what each fix covers, what each test tries

Read it top down. Every length the random test makes lies inside both lower bars, so it cannot tell patch A from patch B. Only the ticks at 512 and 1023 fall in the gap.

Do it: both patches, then the run that splits them

Both pass what failed

$ grep -n "localparam int CW" cw9/rtl/dma_engine.sv cwfix/rtl/dma_engine.sv
cw9/rtl/dma_engine.sv:36:  localparam int CW = 9;           // beat counter width
cwfix/rtl/dma_engine.sv:36:  localparam int CW = $bits(cfg_len); // beat counter width: as wide as the length it counts
$ cw9/tb/run.sh random 10
dbglab: test=random seed=10
PASS test=random seed=10 cycles=21581 time=215806 ns
$ cwfix/tb/run.sh random 10
dbglab: test=random seed=10
PASS test=random seed=10 cycles=21581 time=215806 ns

Same cycle count, to the cycle. Seed 10 cannot tell them apart, and neither can Verilator, which gives the same two lines.

Both pass the sweep

$ for v in head cw9 cwfix; do (cd $v; for s in $(seq 1 60); do tb/run.sh random $s | grep FAIL; done > ../sweep60-$v.txt); echo "$v: $(wc -l < sweep60-$v.txt) fail, $(grep -c xxxxxxxx sweep60-$v.txt) with X"; done
head:       19 fail, 9 with X
cw9:        9 fail, 9 with X
cwfix:        9 fail, 9 with X
$ diff sweep60-cw9.txt sweep60-cwfix.txt && echo same
same

Ten wrong-value failures at HEAD, none with either patch. The nine left show an X, which is a different bug (B4), and they are line for line the same with both patches. If the review stops here, the patches are equal and the shorter one wins.

The test where they disagree

The disagreement is at lengths from 512 up. The register map, not the random test, says those are legal, so write the test from the register map. Name it for the rule it checks, not for the bug that prompted it:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

// not-run: a task for dbglab's bench (tb/tests.svh); it needs the bench's helpers
// dma_len_limits: LEN is 10 bits, so every length from 0 to 1023 is legal (register map).
// Copy at the edges of each power of two that fits in RAM, then fill 1023 words.
task automatic t_dma_len_limits();
  logic [31:0] st;
  int len;
  csr_wr(CSR_CTRL, 32'h0);
  for (int k = 0; k < 4; k++) begin
    case (k)
      0: len = 255;
      1: len = 256;
      2: len = 511;
      default: len = 512;
    endcase
    run_copy(0, 512, len);
    for (int i = 0; i < len; i++) ram_check(512 + i);
  end
  csr_wr(CSR_FILL, 32'hA5A5_0000 ^ seed);                          // the longest legal run: a fill
  csr_wr(CSR_CTRL, 32'h1 << CTRL_FILL);
  dma_program(32'h0, RAM_BASE + 4 * 1, 1023);
  dma_wait(st);
  dma_close(st, 1'b0, 32'h0);
  model_copy(0, 1, 1023, 1'b1);
  for (int i = 0; i < 1024; i++) ram_check(i);
endtask

$ head/tb/run.sh dma_len_limits 1
dbglab: test=dma_len_limits seed=1
FAIL test=dma_len_limits seed=1 cycle=4917: SCOREBOARD: RAM word 516 (address 00001810) read 228510de, expected 934c82b1 (last written by DMA run #2)
$ cw9/tb/run.sh dma_len_limits 1
dbglab: test=dma_len_limits seed=1
FAIL test=dma_len_limits seed=1 cycle=15445: SCOREBOARD: RAM word 516 (address 00001810) read 228510de, expected 934c82b1 (last written by DMA run #4)
$ cwfix/tb/run.sh dma_len_limits 1
dbglab: test=dma_len_limits seed=1
PASS test=dma_len_limits seed=1 cycles=24673 time=246726 ns

Read the three results together:

• HEAD fails on run #2, the 256-word copy, at cycle 4917: an 8-bit counter loads 0 for 256.

• Patch A gets past runs #1 to #3 (255, 256 and 511 words) and fails on run #4, the 512-word copy, at cycle 15445. The same failure, on the same word, one power of two later.

• Patch B passes all five runs, including the 1023-word fill. Seeds 2 and 3 and Verilator give the same three verdicts.

A probe on patch A's run #4 shows the mechanism, and why the bench needed a data check rather than a length check:

$ probes/run-probe.sh cw9 ../probes/dma_probe.sv dma_len_limits 1 | grep "run #4"
cycle 11329: DMA run #4 start  LEN=512 left_q=0
cycle 15421: DMA run #4 done   words written=512
FAIL test=dma_len_limits seed=1 cycle=15445: SCOREBOARD: RAM word 516 (address 00001810) read 228510de, expected 934c82b1 (last written by DMA run #4)

left_q loads 0, the chunk logic misbehaves, and the run still writes 512 words, of the wrong data. A check that only counted words would have passed patch A.

Close the door behind the fix

Patch B keeps the CW'(cfg_len) cast, which is now a no-op. Remove it. Without the cast, a future narrowing is a lint warning instead of a silent wrap:

$ verilator --lint-only -Wall --top-module dma_engine ../cw9/rtl/dbglab_pkg.sv ../cw9/rtl/sync_fifo.sv dma_cw9.sv 2>&1 | grep WIDTH | grep dma_
%Warning-WIDTHTRUNC: dma_cw9.sv:89:19: Operator ASSIGNDLY expects 9 bits on the Assign RHS, but Assign RHS's VARREF 'cfg_len' generates 10 bits.
$ verilator --lint-only -Wall --top-module dma_engine ../cwfix/rtl/dbglab_pkg.sv ../cwfix/rtl/sync_fifo.sv dma_cwfix.sv 2>&1 | grep WIDTH | grep dma_; echo "exit $?"
exit 1

The derived width prints no warning (grep finds nothing and exits 1); patch A without its cast would have been flagged at once. The cast was part of the cause: it told the tool the truncation was intended.

What the evidence says

Patch A was a real change at the fault, in the right line, and it was still the wrong fix. It moved the wrap from 256 to 512, beyond every length the tests make, so the regression could never fail again. That is worse than no fix: the bug is now rarer, and its ticket is closed. Patch B removed the decision that made the bug possible. The run that separated them took a second, and it was found by asking one question: what range must this hold for?

The first row is the most common symptom fix of all, and it was run too: one line in the random test, if (r_len > 255) r_len = 255;, makes seed 10 pass and the sweep clean while the design is untouched. The limits test fails on it exactly as it fails at HEAD, because the test programs LEN itself. The debug loop names this as a trap; here it is measured.

Traps

Related topics

• Part 9 · Fix, Prove, Prevent

• Next: proving the fix

• Symptom, fault, cause (a symptom patch on B4)

• One change at a time

• Case study: a fix that looks right and is not