From c1b3c4c1561d6b2df04a0923659e2283162e8a01 Mon Sep 17 00:00:00 2001 From: Anachronaut Date: Tue, 1 Sep 2026 15:43:25 -0400 Subject: [PATCH] A rule for the bug that formatted a disk falls-into-subroutine. The code above a label ends without going anywhere and the label is one something CALLs, so execution walks into the subroutine, reaches its RET, and returns to whatever the Stack happens to hold - because nobody called, there is no caller, and it goes somewhere nobody named. It is worth a rule because the symptom is nowhere near the cause and changes with the Stack. In CosmOS's monitor it was usually a byte that does not decode, in the middle of newLine; once it was inside sbfsFormat, and the machine formatted the disk it had booted from. Two exemptions, and both had to exist or the rule would have reported well written code: A TAIL CALL IS THE SAME SHAPE AND IS FINE. Falling out of one subroutine into another means the RET returns to the outer caller, which is real. So it only fires when nothing since the last branch or return was a call target either - which is the linter's usual trade of precision for being worth reading. AND osExit NEVER RETURNS. It is how a loaded program gives the machine back, and every program here ends with it and then writes its helpers underneath. Without that, twelve well written programs were reported. It is the one name from the system this tool knows, and the comment says why it is there. Also SRET, which stopsFallthrough did not list. It returns from a handler exactly as RET returns from a call, and leaving it out is a gap in every rule that asks what reaches an instruction. Load bearing rather than tidy: without it cosmos.asm reports a handler ending in SRET as falling into the routine written under it. A first pass over the file collects call targets, because a subroutine is very often called from further down than it is written. The corpus reports none of it, which is the point rather than a disappointment, and the Test Manual now says so - a baseline entry that is absent is otherwise indistinguishable from a rule that never runs. Checked against the version of cosmos.asm from before the fix, where it names the line. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01E2JrLzFvuFX9fgi1LDRjrW --- Source/Linter/Linter.c | 114 +++++++++++++++++++++++++++++++++++++++- SplitBit Test Manual.md | 7 +++ Tests/lint.sh | 45 ++++++++++++++-- 3 files changed, 160 insertions(+), 6 deletions(-) diff --git a/Source/Linter/Linter.c b/Source/Linter/Linter.c index 3f33ee7..1dca057 100644 --- a/Source/Linter/Linter.c +++ b/Source/Linter/Linter.c @@ -23,7 +23,7 @@ static const char *const ruleNames[] = { "redundant-setd", "redundant-assignment", "pointer-offset", "redundant-ccf", "known-branch", "zero-load", "dead-assignment", "q-through-stack", "self-push-pop", - "branch-to-next", "unreachable", "dead-suppression", + "branch-to-next", "unreachable", "dead-suppression", "falls-into-subroutine", }; #define RULE_COUNT ((int)(sizeof(ruleNames) / sizeof(ruleNames[0]))) @@ -329,7 +329,76 @@ static int stopsFallthrough(const SourceInstruction *instruction) { || strcmp(instruction->mnemonic, "RET") == 0 || strcmp(instruction->mnemonic, "RRET") == 0 || strcmp(instruction->mnemonic, "RETI") == 0 - || strcmp(instruction->mnemonic, "HALT") == 0; + // SRET was missing here, which is a gap in every rule that asks what reaches an + // instruction: it returns from a handler exactly as RET returns from a call. + || strcmp(instruction->mnemonic, "SRET") == 0 + || strcmp(instruction->mnemonic, "HALT") == 0 + // ---- The one trap that does not come back ---- + // + // A SWI is a call and almost every one of them returns, so a SWI does not end a run + // of code. osExit is the exception and the exception matters: it is how a loaded + // program gives the machine back, so nothing after it runs, and every program on + // this machine ends with it and then writes its helper routines underneath. + // + // NAMED RATHER THAN NUMBERED, because that is what the source says. This is the only + // name from the system that this tool knows, and it is here because without it the + // rule below would report every well written program in the repository. + || (strcmp(instruction->mnemonic, "SWI") == 0 + && strcmp(instruction->operand, "osExit") == 0); +} + +// ---- Which labels are entered by a CALL ---- +// +// Collected in a pass of its own before anything is judged, because the rule below asks a +// question about a label that cannot be answered until the whole file has been read: a +// subroutine is very often called from further down than it is written. +#define CALL_TARGET_CAPACITY 2048 +static char callTargets[CALL_TARGET_CAPACITY][TOKEN_CAPACITY]; +static int callTargetCount = 0; + +static int isCallTarget(const char *name) { + for (int i = 0; i < callTargetCount; i++) { + if (strcmp(callTargets[i], name) == 0) { + return 1; + } + } + return 0; +} + +// Reads the file once for nothing but CALL and RCAL operands. Answers 0 if there were more +// distinct ones than there is room for, which is reported rather than quietly truncated: a +// rule that silently forgot half its input would go quiet instead of wrong, which is worse. +static int collectCallTargets(const char *path) { + callTargetCount = 0; + FILE *file = fopen(path, "r"); + if (!file) { + return 1; // The caller opens it again and reports the failure properly. + } + char line[LINE_CAPACITY]; + int lineNumber = 0; + while (fgets(line, sizeof(line), file)) { + lineNumber++; + SourceInstruction current; + if (parseInstruction(line, lineNumber, ¤t) != SOURCE_INSTRUCTION) { + continue; + } + if (strcmp(current.mnemonic, "CALL") != 0 && strcmp(current.mnemonic, "RCAL") != 0) { + continue; + } + if (!current.operand[0] || isCallTarget(current.operand)) { + continue; + } + if (callTargetCount >= CALL_TARGET_CAPACITY) { + fclose(file); + fprintf(stderr, "%s: error: more than %d distinct call targets\n", + path, CALL_TARGET_CAPACITY); + return 0; + } + snprintf(callTargets[callTargetCount], TOKEN_CAPACITY, "%s", current.operand); + callTargetCount++; + } + fclose(file); + return 1; } static int isDirectBranch(const SourceInstruction *instruction) { @@ -699,12 +768,21 @@ static int lintFile(const char *path) { return -1; } + if (!collectCallTargets(path)) { + fclose(file); + return -1; + } + char line[LINE_CAPACITY]; int lineNumber = 0; int warnings = 0; int havePrevious = 0; int fallthroughStopped = 0; int havePendingBranch = 0; + // Whether anything at all has been read since the last #Program or #Data, and whether + // the run of code we are in began at a label something calls. See the rule below. + int sawInstruction = 0; + int runHasCallTarget = 0; SourceInstruction pendingBranch; KnownRegisters known = {0}; KnownPointer pointers[DATA_POINTERS] = {0}; @@ -751,6 +829,33 @@ static int lintFile(const char *path) { "remove the branch"); havePendingBranch = 0; } + // ---- Walking into a subroutine instead of calling it ---- + // + // The code above ends without going anywhere, and the label below is one + // something CALLs. So execution walks into the subroutine, reaches its RET, + // and returns to whatever the Stack happens to hold - which is not a caller, + // because nobody called. It goes somewhere nobody named. + // + // THAT IS WHAT FORMATTED A DISK. CosmOS's monitor had no branch at the end + // of its command list, so an unrecognised word walked into sayPrompt; the + // RET at the bottom of it went to whatever was on the Stack, and one of the + // places that turned out to be was inside the filesystem's format routine. + // The symptom is nowhere near the cause and changes with the Stack, which is + // exactly the kind of fault worth spending a rule on. + // + // UNLESS THE RUN WE ARE IN WAS ITSELF CALLED. Falling out of one subroutine + // into another is an ordinary tail call: the RET returns to the outer + // caller, which is real and is what the author meant. So this only fires + // when nothing since the last RET or branch was a call target either. + if (!fallthroughStopped && sawInstruction && !runHasCallTarget + && current.spelling[0] && isCallTarget(current.spelling)) { + warnings += warning(path, "falls-into-subroutine", current.line, + "execution walks into a subroutine nothing here called", + "branch past it, or end the code above with a branch or a return"); + } + if (current.spelling[0] && isCallTarget(current.spelling)) { + runHasCallTarget = 1; + } fallthroughStopped = 0; forgetRegisters(&known); forgetPointers(pointers); @@ -758,6 +863,8 @@ static int lintFile(const char *path) { } else if (sourceLine == SOURCE_BOUNDARY) { fallthroughStopped = 0; havePendingBranch = 0; + sawInstruction = 0; + runHasCallTarget = 0; forgetRegisters(&known); forgetPointers(pointers); carry = CARRY_UNKNOWN; @@ -778,8 +885,11 @@ static int lintFile(const char *path) { warnings += lintKnownPointers(path, ¤t, pointers); warnings += lintKnownCarry(path, ¤t, carry); warnings += lintInstruction(path, havePrevious ? &previous : &empty, ¤t); + sawInstruction = 1; if (stopsFallthrough(¤t)) { fallthroughStopped = 1; + // A new run of code starts after this, and it has not been called yet. + runHasCallTarget = 0; } if (isDirectBranch(¤t) && current.operand[0]) { pendingBranch = current; diff --git a/SplitBit Test Manual.md b/SplitBit Test Manual.md index a66dbea..e6f5646 100644 --- a/SplitBit Test Manual.md +++ b/SplitBit Test Manual.md @@ -446,6 +446,13 @@ than reading all of them: file across the whole corpus - 33 file-and-rule pairs. It is checked on every run, and it is checked **in both directions.** +The corpus holds none of `falls-into-subroutine`, and that is worth saying rather than +leaving as a gap in the file. It is the rule added after a bug in CosmOS's monitor walked +into `sayPrompt`, reached a `RET` that had no caller, and returned into the filesystem's +format routine - which formatted the disk the machine had booted from. A rule whose count is +zero everywhere is not a rule doing nothing; it is the shape of fault that is worth never +having again. + A new warning appearing is a regression. A recorded warning *disappearing* is also reported, and that is the half people do not expect: it means either that somebody fixed something and did not record it, which is fine and takes one command, or that a rule diff --git a/Tests/lint.sh b/Tests/lint.sh index 05b3759..f76bc9f 100755 --- a/Tests/lint.sh +++ b/Tests/lint.sh @@ -116,6 +116,27 @@ printf '%s\n' \ ' INIA 0d9' \ ' CALL somewhere' \ ' INIA 0d9' \ + ' RET' \ + ' ; ---- Walking into a subroutine instead of calling it ----' \ + 'walksInto:' \ + ' INIA 0d1' \ + 'somewhere:' \ + ' RET' \ + ' ; ---- And a tail call, which is the same shape and is fine ----' \ + 'tailCaller:' \ + ' INIA 0d2' \ + 'alsoCalled:' \ + ' RET' \ + ' ; ---- And osExit, after which nothing runs ----' \ + 'stopsHere:' \ + ' SWI osExit' \ + 'calledOnly:' \ + ' RET' \ + 'theCallsThemselves:' \ + ' CALL tailCaller' \ + ' CALL alsoCalled' \ + ' CALL calledOnly' \ + ' CALL stopsHere' \ > "$fixture" "$LINT" "$fixture" > "$output" 2>&1 @@ -162,6 +183,13 @@ expect 53 "carry is already known to be clear" expect 56 "BRC is always taken because carry is known set" expect 60 "BRC is never taken because carry is known clear" expect 61 "BNC is always taken because carry is known clear" +# ---- Walking into a subroutine instead of calling it ---- +# +# The code above ends without going anywhere and the label below is one something CALLs, so +# execution reaches a RET that has no caller and returns to whatever the Stack held. That is +# what formatted a disk: CosmOS's monitor had nothing at the end of its command list, so an +# unrecognised word walked into sayPrompt and its RET went into the filesystem's formatter. +expect 78 "execution walks into a subroutine nothing here called" # ---- And a claim does not survive a call ---- # @@ -173,14 +201,23 @@ expect 61 "BNC is always taken because carry is known clear" # # THE LINE NUMBERS ARE THE REPEATS, not the label above them. Checking the label instead # passes for free, because a label is never warned about by anything. +# ---- The two shapes that look the same and are not ---- +# +# 82 is a TAIL CALL: the run above it began at tailCaller, which is itself called, so the RET +# at the bottom returns to that caller and everything is where it meant to be. Falling out of +# one subroutine into another is an ordinary thing to do and must stay quiet. +# +# 86 follows osExit, which is how a program gives the machine back and never returns. Every +# well written program on this machine ends that way and then writes its helpers underneath, +# so a rule that did not know it would report the entire repository. quiet=0 -for line in 70 73; do +for line in 70 73 82 86; do if grep -q "^${fixture}:${line}: style:" "$output"; then - FAIL=$((FAIL + 1)); MISSING+=("line $line should be quiet across a CALL") - printf " [%sFAIL%s] line %-3s no claim survives a CALL\n" "$RED" "$RESET" "$line" + FAIL=$((FAIL + 1)); MISSING+=("line $line should have stayed quiet") + printf " [%sFAIL%s] line %-3s should have stayed quiet\n" "$RED" "$RESET" "$line" else PASS=$((PASS + 1)); quiet=$((quiet + 1)) - printf " [%sok %s] line %-3s no claim survives a CALL\n" "$GREEN" "$RESET" "$line" + printf " [%sok %s] line %-3s stays quiet\n" "$GREEN" "$RESET" "$line" fi done