From d4e5c26fd602fa2e67de3fe86f79d86911a5c18e Mon Sep 17 00:00:00 2001 From: Scott Duensing Date: Mon, 29 Jun 2026 17:37:45 -0500 Subject: [PATCH] More bug fixes. --- demos/rsrcProbe.apl | Bin 46685 -> 46685 bytes runtime/build.sh | 9 +- runtime/src/libc.c | 28 +++--- scripts/smokeTest.sh | 88 ++++++++++++++++++ src/link816/omfEmit.cpp | 10 +- .../lib/Target/W65816/W65816BranchExpand.cpp | 31 +++++- 6 files changed, 144 insertions(+), 22 deletions(-) diff --git a/demos/rsrcProbe.apl b/demos/rsrcProbe.apl index e1becc3a61957d20a11a7c6af1c0a94c7a093898..8329fe5ccfcccde915dedf7fdc81610c290fc293 100644 GIT binary patch delta 3230 zcmb`IdrVYU9>>p_86HBV6m)n9It~lWpd*g~sn{Y=-Y=^Lq+_Bo_s%F{HjTTsjg1bO zyQ3R6H0r?;h^sEsL7~OMUfbPeBb)FCS*$UQfnB1qiz7}c5=!wAODpW}oHOHWHrfAr zC*S)yzu)(Fe&^gf_s&erDkf$XeJS{jk80eSii3_p@oz&K_@s3*1zsz8MR7opw1pg1G~h?_zZEf0yCF?$OIu+V`rQ$$T~Wd{m?oyGdVc3Us@)E|;BuMXqB)O{Jg+C#Rl#L1RtDJfCzDZ#^DHQI(Hl;If zL_zPyQN;+RJFX!+QabQ8GMn;?uyacLA;rrHa@>X-1uO^-`Oq+fJBW3YfZvvHY?{Z~ zGrQsYmSUQvHQi>bv<{vE=Q_t^v%~F-fk&rpvRfqg(&B8ttN66#hg7$wx#h>yAGMg;B!`iWAiTbA+wJAtQh~K zbIfV;InjRSbLRB>9JSvWVa``RC(7?UV9rCIqY(Vo3uZ0*ES2AhN~d=rCf$1%Li|oT zbBsPG(C<_*r;0jceTEtLliy^Vh3_iLSRULOgq+IghP7nYxS3ou7U2o<(0Bq$#LT0( zmE6gEzP=#~Ndfe+2yzCLp{5bk1VNwDUWJ~hkuyl!FF(#|T7gH%ueYUOGr71e3*M@+ zZNI`WzbwCbh1FWx^|kX&Z8o`=9}S(K=5K^zZ9xVUe^+cE_JRr+*$NHh$AUB7+`wBj zM_pqfcL%*SjYYnilwx0vlaA!?iceuYPTns)m@uNFKQQo?!LxfGIrTlx4O+O;?w^(= zgEPCwxkWoc6lE?vAzvw*!_c{{;v$^0E9olPl?IYfMOSI7Fc5du5pT|3O>^MN`P+3t z_#Qb_o2hQz<8ZqsUw@zqSCuU<$A2 zt#f&b=Jcq^v8b6fX`Yy%z_mE9)cj}<0}qWa(Tk( zwEeC*VC)|GpL@Ouwhiw?=oZKWLi-U42kAgtK{cSmpcA08AQxyDGz#*7R02W^z!3Nc zQG;}#Y)}cP7IXmQ0JVdzgKmQEf*ymA8RnSbjZTC?I%p?|HhkAh33wjV=Z&fN^9z1a ze(t8XVZ%o(2%QF91xc`R$Ny90e;GFKLHey~a>*JjAGWrY+ny?s0kzZsa!{uMkca9K zpa`9&$!^pY0{AP^ZU;O0|O)TTt=tZSmB*ELp(&h;G>f7h<81AL!2UJIB{j;6_vbu2li zzeb~ijcmdW1Do)L;cdEnSL&JFfXnG@rJM|_5h6!MYY-sVI1vUoWn|t9BTd@Sv&@Dp z$f~ngHaFL(gs3fdL%8g9DM~W(!a&f z$x&a(B25uPJBKZzn1vlB>_R(B*z(F!)-6P*5hK+~!INZYUJ}^DPEs7bzLop|y{eT4_oZ^|FqgWKS zQY;BI6bkV$MWA?sB1Ak(5hl7QBE(?|wKz%SSNOcF!t0V$%IVw0$& z$PlwBHj5<`IbtnEu6Tf=Ky*+Pi|rKU;&qB@@g~Ji@h(NJ_?W^ZA~RsGsHQNBi4;~b zo#K$V(?bz%keQs2ge1DPL3{)9*(OV ze+1C??@JyIa13$`aSU_Z;JC$co8uoGcR2pV;pXsg%y7(cEO0#ISfqgWZ;6Kr3;Rj} zIYKzXI3hUI9I+g0IkX%}94Q=|I5I4^>0R4wIbs3iSWZ*qTCP$QSR{A^-ay3`2YnRE YIjT8!a@2B|IQDXwIjj~3S^Pry4@Kp8IRF3v delta 3106 zcmb`|dr(x@9S87pc2__|0;s&?ab1G2z{2tnl}6d+DGxEN!^4G<$Gxl|*iOl4^O$ih z+f`;89P~IEZN@h2EDBT5cw1}L0f$;z(u|XWp^g>`D(f_i} zZ$J0VIy~~2*&sF2kRafKiYlWq5p{T*$FAWR|@Xyd0ZJCU=T-|Vd@)rnwIBCD} zP+L%nmS*8~Y-Z4)J*!9XUW?E!Z85Q6bNF|wUHv`XIn~e#Y&qJ)g& zcnz$*PJC1cRPJr$u&NPXlK-X(!L(aaC*bXJjOGaTmPTCn)w%_B>p$dQt}bC`Tck#UIo4lCkcX@u7=L=ieu$zZ& zCDgJIS#0aN5!`}J2NytcN03g9$H>jlI6Njl32nsq0ofCl@7EyOHuhRXTTE}2Xbb5z zldiCQ4@tE7_tufcu+1JdqHRs@ei9je5l-k2pT_gDGeT0~D&n>2Q=CU8HcjIe`FGlA ztQszt=b~cN_>3G7vy8EZ`0HE{k{i0cxRdzAB}5(8IE`NPu6$!{Yv5^jsGY`P6@BhE zRKpm-C%l(b#$CW|B! zPuCXWux;#)u-GEuXYP!&Pde;wX9#>z&P{d;WY!R7ZU1|cTX3{LOBxikm(dg!5WJe& zS6GS-JoQu34F)QlCzl9Nyn4 z?q~0Ma$q;T%A+DZ*<8}1WJ4u1d#6}0+xNtJ;%3rUmKK;-X?MFOw>{SQX$n?WLcM!j z?rs0dBVUyrG#n{&%{AF)oErPA0LRapgzPdnxIgEb*QwCL>IOI=xDlhK2EIa0mtUg$ z&BA_9roY*5mhQKw;+b%&ylc)i+hm_S?uI?5ogVfXn$6RlGb@Bs#Jn#`Ll5L1@AJmE zgS>0Ds5&Z0d}RzYVU_7?J1T@%%&os!7U%(~uBZzBtwHhG@R43GJsE?thp}vIj zuR$mlDg&wnsurphsspMM>P{U(ec%w(2-GyxB9t2b$_GG2L7AXRq27XOgE|Lw1?o1` zNG-CWDLD8H$`>}H8zv}bf)!BBN>A6m{jz@ho1TOlO{_<#80sKYrU;+H|0}Pr7vvL` zJB6nY1mvP(8g`@iy@6Y3q6Bz`UN-`Jv3ncPk1wSG58!`D zz(c(~8Q8Jr5IyfNuNVx#PyN`|Aps0|JzFDT50YT*&G7l87gjYIkF27jvXDo4Z&M4U zUprh5JdHk62F%20X!vd{3;(2Rqg{FeJ0VxkPUzPA(92&)+D*?Bli8JCCTmlnGo?5@ zfx9WAzCd;=Tl;M)4Xx-}TJ#o3N77ijv?Emw-QJx8vB1%t6=Yf8 zF0qr+Bi^R;i62w0iPMzp;+K@0qJIt0FUC>^#SF@jSVH+otfkx&TPY)A2W3p`q&yVw z)BzKswU4%LafmV_j!@>rY0A8~Nclok*8(ra0LqdWMOhY26qQs;@sQr4cuQ>*U+Ekr zK)ON+lx|Z(r4dSmG)2)$pHVhTzE*l0aT2?mL@9xGDZERUGHI6~RZwcZGr@6lW;k=4dFdqm)V`1s>w%Y2F=a_QNLiLL h#TV+=kgDDUA`gx?$Cnep3FL%wA~@Q56IuGT_;0s?Z|ML4 diff --git a/runtime/build.sh b/runtime/build.sh index ef093cb..da397d3 100755 --- a/runtime/build.sh +++ b/runtime/build.sh @@ -84,10 +84,11 @@ asm "$SRC/iigsToolbox.s" # generated iigsToolbox.s so a `python3 scripts/genToolbox.py` regen # doesn't clobber them. See runtime/src/iigsToolboxExtra.s. asm "$SRC/iigsToolboxExtra.s" -# softDouble.c builds at -O2. dpack is noinline to dodge a backend -# stack-slot aliasing bug; dclass stays inline because pointer-arg -# stores from a noinline boundary use DBR-relative addressing (broken -# under DBR != 0). Both choices documented in the source. +# softDouble.c builds at -O2. dpack is a plain inline static now (the +# old stack-slot aliasing bug was fixed in-backend, see +# feedback_dpack_inline_fixed). dclass is KEPT inline because pointer-arg +# stores from a noinline boundary lower to DBR-relative `sta (d,s),y` +# (broken under DBR != 0). That choice is documented in the source. cc "$SRC/softDouble.c" # Phase 6.2 UBSan-min runtime. MUST be compiled with diff --git a/runtime/src/libc.c b/runtime/src/libc.c index fbd4214..1451cdc 100644 --- a/runtime/src/libc.c +++ b/runtime/src/libc.c @@ -542,10 +542,12 @@ void *malloc(size_t n0) { // Heap ceiling is ~32KB so anything > 0x7FF0 is unsatisfiable. if (n0 > (size_t)0x7FF0) return (void *)0; // Round up to 2-byte alignment, with a minimum of FREE_NODE_SZ-HDR_SZ. - // Keep this in 16-bit arithmetic — the 0x7FF0 cap above guarantees the - // value fits. Going through `unsigned long` here triggers an i32 umax - // pattern that our backend currently miscompiles; staying 16-bit dodges - // that path entirely. + // Keep this in 16-bit arithmetic: the 0x7FF0 cap above guarantees the + // value fits, and 16-bit ops are cheaper than i32 on the 65816. (An + // earlier comment here blamed an "i32 umax miscompile" for staying + // 16-bit; that was a misattribution. The i32 path is correct, verified + // in MAME; the real over-heap bug was a control-flow live-in staleness + // bug in W65816BranchExpand, since fixed.) u16 n = (u16)n0; if (n == 0) n = 1; n = (u16)((n + 1) & ~(u16)1); @@ -572,13 +574,17 @@ void *malloc(size_t n0) { link = &cur->next; cur = cur->next; } - // Bump-allocate from the high end. Big allocations (e.g. the 16 KB sprite - // codegen scratch) are routed through halBigAlloc -> the Memory Manager, - // NOT this heap, so n is always small here and the historical - // `p + HDR_SZ + n > heapEnd` over-heap miscompile (only manifested for - // oversized n) is never exercised. A `heapEnd - p` reformulation went - // through the same i32 path and spuriously failed small mallocs, so keep - // the original simple compare. + // Bump-allocate from the high end. This heap only serves small + // allocations: the 0x7FF0 cap at the top of malloc() bounds n, and + // callers needing big buffers (e.g. a 16 KB sprite scratch) get them + // straight from the GS/OS Memory Manager, not malloc. (There is no + // "halBigAlloc" routing -- an older comment here described one that was + // never implemented; oversized requests simply hit the 0x7FF0 cap and + // return NULL.) The `p + HDR_SZ + n > heapEnd` compare is correct and + // is covered by smoke check #176. The historical "over-heap" failures + // were the W65816BranchExpand/SepRepCleanup live-in bug (since fixed), + // not an i32 miscompile; `heapEnd - p` is equally correct now, MAME- + // verified -- the addition form is kept only because it is the tested one. char *p = bumpPtr; if (p + HDR_SZ + n > heapEnd) return (void *)0; *(u16 *)p = (u16)n; diff --git a/scripts/smokeTest.sh b/scripts/smokeTest.sh index f76f524..11185b5 100755 --- a/scripts/smokeTest.sh +++ b/scripts/smokeTest.sh @@ -2894,6 +2894,94 @@ EOF fi rm -f "$cMcFile" "$oMcFile" "$binMcFile" + # Regression: W65816SepRepCleanup's PHI-copy hoist must NOT move a + # NULL-return materialization (lda #0) across the bump bounds-compare + # when the over-heap edge leaves via a BranchExpand-created Bridge + # (BRL) block. Pre-fix that hoist was unsound because the Bridge + # block's live-ins were left empty, so the guard wrongly concluded A + # was dead and the over-heap path returned the compare result instead + # of NULL — letting a drain run off the heap. Fixed by recomputing + # live-ins for newly-created blocks in W65816BranchExpand. This + # self-contained malloc-shaped reproducer (free-list walk + split + + # i32 cap raise the register pressure / force the Bridge block) drains + # a fixed nine-block heap; pre-fix it ran to the 0x40 cap (over-run), + # post-fix it stops at 9. + log "check: MAME malloc-shape drain stops at heap end (#176 SepRep/BranchExpand live-in)" + cBrFile="$(mktemp --suffix=.c)" + oBrFile="$(mktemp --suffix=.o)" + binBrFile="$(mktemp --suffix=.bin)" + cat > "$cBrFile" <<'EOF' +typedef unsigned long u32; +typedef unsigned short u16; +typedef struct B { u16 size; struct B *next; } B; +#define HDR ((u16)sizeof(u16)) +#define MIN_SPLIT ((u16)(sizeof(u16) + sizeof(B *) + 2)) +#define MAX_REQ ((u32)0x7FF0) +#define DRAIN_CAP ((u16)0x40) +static B *freeList; +static char *bumpPtr; +static char *heapEnd; +volatile u32 vStart = 0x2E80; +volatile u32 vEnd = 0xBF00; +volatile u32 vReq = 0x1000; +__attribute__((noinline)) static void *mal(u32 n0) { + if (n0 > MAX_REQ) { + return (void *)0; + } + u16 n = (u16)n0; + B **link = &freeList; + B *cur = freeList; + while (cur) { + if (cur->size >= n) { + if (cur->size >= n + MIN_SPLIT) { + u16 rem = (u16)(cur->size - n - HDR); + B *tail = (B *)((char *)cur + HDR + n); + tail->size = rem; + tail->next = cur->next; + cur->size = n; + *link = tail; + } else { + *link = cur->next; + } + return (char *)cur + HDR; + } + link = &cur->next; + cur = cur->next; + } + char *p = bumpPtr; + if (p + HDR + n > heapEnd) { + return (void *)0; + } + *(u16 *)p = n; + bumpPtr = p + HDR + n; + return p + HDR; +} +int main(void) { + bumpPtr = (char *)vStart; + heapEnd = (char *)vEnd; + freeList = (B *)0; + u16 count = 0; + for (u16 i = 0; i < DRAIN_CAP; i++) { + if (!mal(vReq)) { + break; + } + count++; + } + *(volatile u16 *)0x025000 = count; + while (1) { + } +} +EOF + "$CLANG" --target=w65816 -O2 -ffunction-sections -c "$cBrFile" -o "$oBrFile" + "$PROJECT_ROOT/tools/link816" -o "$binBrFile" --text-base 0x1000 \ + "$oCrt0F" "$oLibgccFile" "$oBrFile" >/dev/null 2>&1 + if ! bash "$PROJECT_ROOT/scripts/runInMame.sh" "$binBrFile" --check \ + 0x025000=0009 >/dev/null 2>&1; then + die "MAME: malloc-shape drain over-ran heap (BranchExpand live-in regression)" + fi + log "OK: malloc-shape drain stopped at the 9-block heap end (no over-run)" + rm -f "$cBrFile" "$oBrFile" "$binBrFile" + log "check: MAME runs strtok 'a,b,,c' continuation (#84 fixed)" cTkFile="$(mktemp --suffix=.c)" oTkFile="$(mktemp --suffix=.o)" diff --git a/src/link816/omfEmit.cpp b/src/link816/omfEmit.cpp index 6736dcd..4e7376e 100644 --- a/src/link816/omfEmit.cpp +++ b/src/link816/omfEmit.cpp @@ -1030,11 +1030,11 @@ int main(int argc, char **argv) { // --manifest + --expressload: wrap N user segments in a single // ~ExpressLoad descriptor (Phase C.1). Each user seg gets // KIND=0x1000 (CODE|PRIV), so the Loader picks banks dynamically - // — programs MUST NOT depend on link-time bank placement of - // cross-segment IMM24 targets (those need cINTERSEG, deferred - // to Phase C.2). Intra-segment cRELOC is supported via the - // global gReloc24Sites (single-sidecar path; per-seg sidecars - // also deferred to Phase C.2). + // — programs MUST NOT depend on link-time bank placement. Both + // intra-segment cRELOC and cross-segment IMM24 targets (via + // cINTERSEG, 0xF6) are handled: Phase C.2 (per-seg sidecars with + // intra + inter site lists) is implemented in the loop below. + // Intra-only single-sidecar builds still use global gReloc24Sites. if (expressload) { std::vector users; users.reserve(segs.size()); diff --git a/src/llvm/lib/Target/W65816/W65816BranchExpand.cpp b/src/llvm/lib/Target/W65816/W65816BranchExpand.cpp index d7958fd..7d118ae 100644 --- a/src/llvm/lib/Target/W65816/W65816BranchExpand.cpp +++ b/src/llvm/lib/Target/W65816/W65816BranchExpand.cpp @@ -42,6 +42,7 @@ #include "W65816.h" #include "W65816InstrInfo.h" #include "W65816Subtarget.h" +#include "llvm/CodeGen/LivePhysRegs.h" #include "llvm/CodeGen/MachineFunction.h" #include "llvm/CodeGen/MachineFunctionPass.h" #include "llvm/CodeGen/MachineInstr.h" @@ -152,7 +153,8 @@ static unsigned estimateDistance(MachineFunction &MF, // sliced after each non-final conditional, so every MBB ends up with // at most one conditional terminator. Returns true if any MBB was // split. -static bool splitMultiBranchMBBs(MachineFunction &MF) { +static bool splitMultiBranchMBBs(MachineFunction &MF, + SmallVectorImpl &NewBlocks) { bool Changed = false; // Snapshot MBBs first (we mutate the list during iteration). SmallVector MBBs; @@ -183,6 +185,7 @@ static bool splitMultiBranchMBBs(MachineFunction &MF) { // Create new MBB; transfer everything after splitAfter to it. auto *NewMBB = MF.CreateMachineBasicBlock(MBB->getBasicBlock()); MF.insert(std::next(MBB->getIterator()), NewMBB); + NewBlocks.push_back(NewMBB); // Move instructions [splitAfter+1 .. end) to NewMBB. auto moveStart = std::next(splitAfter); NewMBB->splice(NewMBB->end(), MBB, moveStart, MBB->end()); @@ -331,6 +334,13 @@ bool W65816BranchExpand::runOnMachineFunction(MachineFunction &MF) { const auto &STI = MF.getSubtarget(); const auto *TII = STI.getInstrInfo(); bool AnyChanged = false; + // Blocks created below (split tails + `BRL Target` bridges). These are + // wired into the CFG with successors but no live-in lists; we recompute + // live-ins for exactly these blocks at the end so downstream passes that + // query successor live-ins (W65816SepRepCleanup) stay sound. Existing + // blocks' live-ins are unaffected by inserting predecessors, so there's + // no need to recompute the whole function. + SmallVector NewBlocks; // Step -1: invert Bxx + BRA when cond target is the next MBB. Saves // 3 cyc per backedge in tight loops (sumOfSquares 50 iters: -150 cyc). @@ -342,7 +352,7 @@ bool W65816BranchExpand::runOnMachineFunction(MachineFunction &MF) { AnyChanged |= dropDeadConditionalsToBRATarget(MF); // Step 1: split multi-conditional-terminator MBBs. - AnyChanged |= splitMultiBranchMBBs(MF); + AnyChanged |= splitMultiBranchMBBs(MF, NewBlocks); // Step 2: iterate to fixed-point. Each expansion adds 3 bytes // (bridge BRA), which may push another previously-OK branch over @@ -414,6 +424,7 @@ bool W65816BranchExpand::runOnMachineFunction(MachineFunction &MF) { MachineBasicBlock *Bridge = MF.CreateMachineBasicBlock(MBB->getBasicBlock()); MF.insert(std::next(MBB->getIterator()), Bridge); + NewBlocks.push_back(Bridge); // Replace successor edges: MBB used to have {Target, Skip}; now // it has {Bridge, Skip}. Bridge has {Target}. @@ -485,5 +496,21 @@ bool W65816BranchExpand::runOnMachineFunction(MachineFunction &MF) { Last->eraseFromParent(); AnyChanged = true; } + + // Bridge/split blocks created above are wired into the CFG with successors + // but no live-in lists, so they are left with EMPTY live-ins even though + // values flow through them to their targets. Downstream peepholes that + // query successor live-in sets (W65816SepRepCleanup's PHI-copy hoist and + // lagged-ptr sink guards both call Succ->isLiveIn(A)) would then wrongly + // conclude A is dead at a block exit and hoist a clobbering def across a + // still-live value. This is the root cause of the malloc over-heap + // miscompile: the over-heap NULL return (A=0) lived out only through a + // Bridge block, so the hoist replaced it with the bounds-compare result. + // Recompute live-ins for exactly the new blocks (their successors are + // existing blocks whose live-ins are already correct; fullyRecomputeLiveIns + // converges across chained new blocks). Existing blocks are untouched, so + // this stays O(new blocks) instead of O(function) per invocation. + if (!NewBlocks.empty()) + fullyRecomputeLiveIns(NewBlocks); return AnyChanged; }