https://gcc.gnu.org/bugzilla/show_bug.cgi?id=126299
Bug ID: 126299
Summary: [pdp11] SImode/DImode signed comparisons branch on the
wrong flags whenever the high words are equal
Product: gcc
Version: 13.3.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: target
Assignee: unassigned at gcc dot gnu.org
Reporter: migraineman33 at gmail dot com
Target Milestone: ---
This one was found by Colossal Cave Adventure. Porting the 1977 game
to Fuzix on a homebuilt FPGA PDP-11, its first lseek() past 32K of the
46KB adventure database returned EINVAL: the kernel's 32-bit
`if (p < 0)` range check, compiled by pdp11-aout-gcc, rejects every
offset in 32768..65535. Three weeks of kernel bring-up never tripped
it because nothing else had ever seeked past 32K. The root cause is
general:
config/pdp11/pdp11.md, define_insn "cmpsi" ("Two word compare") emits
a multiword compare as:
cmp/tst <high words>
bne 1f
cmp/tst <low words>
1:
and the following conditional branch consumes the final condition
codes as if they summarized the full 32-bit comparison. That is true
for Z (equality) and — because a carry/borrow out of the low words is
what the high-word compare already accounted for — for the UNSIGNED
conditions. It is NOT true for the signed conditions: when the high
words are equal, the decision falls to the low words, which are an
UNSIGNED quantity; the branch that follows (blt/bge/bgt/ble) instead
interprets the low-word cmp/tst's N and V as the sign of the whole
value.
define_insn "cmpdi" ("Four word compare") has the same construction
and the same defect. The pattern is unchanged in current master.
Reproducers (attached as sicmp_repro.c; pdp11-aout-gcc -Os, EIS
default; -m10 emits the same compare/branch sequences):
1. compare with zero — the Fuzix lseek shape
extern void neg(void);
void f1(long p) { if (p < 0) neg(); }
_f1:
tst 02(sp) ; high word
bne L_3
tst 04(sp) ; high==0 -> test LOW word
L_3: bge L_1 ; branches on LOW word's N!
jsr pc,_neg
L_1: rts pc
Concrete failure: every p in [32768, 65535] (high word 0, low word
bit 15 set) calls neg() — f1(40977) believes 40977 < 0. In the wild:
Fuzix _lseek's `if (p < 0) goto bad;` returned EINVAL for those
offsets on real hardware (and _ftruncate likewise rejected those
lengths). Values with a nonzero high word are handled correctly, so
files/quantities under 32K mask the bug completely.
Second independent wild sighting, userland this time: ubasic's loader
guard `if ((st_size|3) >= ~(size_t)0)` — off_t against 16-bit size_t
promotes to a signed SImode compare — emitted
tst <high word>
bne 1f
cmp <low word>,$-2 ; 65534
1: ble ok
so a 603-byte program compared as 603 > -2 = "File too large" and the
interpreter refused every file, while files of 32768..65531 bytes
(low word negative) would have slipped through the guard. Two
unrelated programs, same silent miscompile shape.
2. two-variable signed compare — wrong in both directions
extern void hit(void);
void f4(long a, long b) { if (a < b) hit(); }
_f4:
cmp 02(sp),06(sp) ; high words
bne L_10
cmp 04(sp),010(sp) ; highs equal -> LOW words
L_10: bge L_8 ; signed branch on low-word N^V
jsr pc,_hit
L_8: rts pc
Concrete failures (high words equal, low words straddling 0x8000):
f4(32768, 1): cmp 0x8000,0x0001 -> 0x7fff, V=1 -> N^V=1 -> claims
32768 < 1.
f4(1, 32768): cmp 0x0001,0x8000 -> V=1,N=1 -> N^V=0 -> claims
1 >= 32768.
The unsigned variant
void f5(unsigned long a, unsigned long b) { if (a < b) hit(); }
emits the same cmp/bne/cmp skeleton with `bhis` and is CORRECT for
all inputs (low-word decision is properly unsigned; high-word C is
valid when the bne fires), confirming the defect is specific to the
signed conditions.
The value form `int f3(long p) { return p < 0; }` does not use cmpsi
(shift-based sign extraction) and is correct — only branches break.
Why no single branch can work: after the composed sequence, one
branch condition must serve two different deciders — signed semantics
when the high-word compare fell through the bne, unsigned semantics
when the low-word compare set the final flags. No single PDP-11
branch condition has both meanings, so the fix has to restructure the
emitted code rather than pick a better branch. The usual approaches
(per other multiword-compare ports):
a) branch tree: signed branches directly off the high-word compare
(blt/bgt decide, bne falls through to the low words), then
UNSIGNED branches (blo/bhis) off the low-word compare; or
b) for compare-with-zero, drop the low-word test entirely for
LT/GE — the high word alone carries the sign; or
c) bias both operands by 0x8000 in the high word and use the
(correct) unsigned path.
Any fix must also keep pdp11_cmp_length and the cc-mode bookkeeping
honest, and the cmpdi pattern needs the same treatment.
Unlike our earlier shift-expander reports (PR125058/126190/126191),
no patch is attached, deliberately: those were one- and two-line
expander fixes we could exhaustively test; this one restructures the
emitted sequence for every multiword signed compare and touches the
length/cc bookkeeping, so it deserves more scrutiny than we can give
it from the outside. Happy to test candidate fixes on our hardware
and to cut a patch if that's useful — but we didn't want to lead
with one.
Workaround used in Fuzix (kernel commit 21c9f1053, ubasic commit
f4061445a on the pdp11-port branch):
if ((int16_t)(p >> 16) < 0) /* instead of: if (p < 0) */
compiles to `ashc $-16` + `tst` of the true high word. Note that the
plausible-looking `(uint32_t)p & 0x80000000UL` does NOT work as a
workaround: GCC canonicalizes it back to `p < 0` and emits the same
miscompiled pattern.
Suggested testsuite shape: execution test running f1 over {4530,
32767, 32768, 40977, 65535, 65536, -1} and f4 over the straddle pairs
above; scan-assembler for whatever the fixed sequence becomes.
