Fix the build: measure.c, the Makefile targets, and an LTO type mismatch - #18
Open
Roxxik wants to merge 2 commits into
Open
Fix the build: measure.c, the Makefile targets, and an LTO type mismatch#18Roxxik wants to merge 2 commits into
Roxxik wants to merge 2 commits into
Conversation
measure.c did not compile under the shipped -Werror flags: the `= 0` in `uint8_t end_a, end_x, end_y, end_s = 0, end_p = 0;` binds only to end_s and end_p, so GCC reported -Wmaybe-uninitialized for the other three. The reads are in fact always preceded by writes, but GCC cannot prove it and the set of reported variables shifts with -O level. Initialise all five. Makefile and Makefile.apple1basic named their link targets after existing directories. Make stats the directory, whose mtime is bumped whenever an object file is written into it, and declares the target up to date. For cbmbasic this silently skips relinking -- including after a failed link, which then reports "Nothing to be done for 'all'" on every retry and can leave a stale binary in place. For apple1basic it was fatal: `-o apple1basic` cannot write to a directory. Name each target after the file it produces and mark the phony targets as such. Fixing the apple1basic output path exposed a second defect the link error had been masking: netlist_sim.o was missing from its OBJS, so the link failed with undefined references to recalcNodeList, readNodes, isNodeHigh and destroyNodesAndTransistors. Add it. All three targets now build clean from scratch, measure characterises all 256 opcodes, and deleting a binary triggers a relink.
glue.h declares `extern unsigned char V, B, D, I, C, N, Z;` while runtime_init.c defines `int N, Z, C;`. runtime_init.c does not include glue.h, so no single translation unit sees both and the conflict is invisible to a normal build. Under -flto GCC compares declarations across translation units and reports -Wlto-type-mismatch for all three, which -Werror promotes to a link failure. Every value stored in these flags is 0 or 1 -- `N = P >> 7`, `Z = (P >> 1) & 1`, `C = P & 1` in runtime_init.c, the SETZ/SETSZ/SETNC macros in glue.h, and the plain `C = 0` / `C = 1` assignments throughout runtime.c. So unsigned char is the correct width, and it matches how A, X, Y and S are already declared and defined. Narrow the definition to match the header rather than widening the header. Verified behaviour-preserving: cbmbasic --benchmark output is byte-for-byte identical to a pristine build of master, both with and without -flto (33155 half-cycles, same final CPU state).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four unrelated build defects, all pre-existing, found while trying to benchmark
the simulator. Reproduced on GCC 16.1.1 / GNU Make 4.4. Self-contained, it
touches no simulator code beyond one declaration in
cbmbasic/.measure.cdoes not compile under the project's own flagsThe
= 0inbinds to
end_sandend_ponly. It is not a live bug, every read is precededby a write on the
j == 0pass, and the guarding condition depends only onk,which is fixed by the enclosing loop. GCC cannot prove that, and which of the
three variables it names shifts with the optimisation level, so it can look
intermittent. This initialises all five rather than suppressing the warning.
Different opt levels shift this. On -O0 there is no warning.
Make targets named after directories that already exist
Makefilebuilds a target calledcbmbasic, andcbmbasic/is a directory.Make stats the directory, whose mtime is bumped whenever an object file is
written into it, and declares the target up to date:
make clean && makealways works and touching a source file does trigger arelink, so this stays invisible day to day. It shows up after a failed link:
every subsequent
makereports "Nothing to be done for 'all'", which reads as asuccessful no-op, and a stale binary can sit there while make insists the tree is
current.
Makefile.apple1basichas the same collision, and there it is fatal, because itlinks with
-o apple1basic:This names each target after the file it actually produces and marks the real
phony targets as such.
.PHONY: cbmbasicwould also stop the misfire, but byforcing a relink on every invocation.
apple1basicis missingnetlist_sim.oFixing the output path above uncovers a second defect the link error had been
masking:
netlist_sim.ois absent from itsOBJS, so the link fails withundefined references to
recalcNodeList,readNodes,isNodeHighanddestroyNodesAndTransistors. That target cannot have linked sincenetlist_sim.cwas split out.N/Z/Care declaredunsigned charand definedintcbmbasic/glue.hdeclaresextern unsigned char V, B, D, I, C, N, Z;whilecbmbasic/runtime_init.cdefinesint N, Z, C;.runtime_init.cdoes notinclude
glue.h, so no single translation unit sees both and an ordinary buildcannot notice. Under
-fltoGCC compares declarations across translation units,reports
-Wlto-type-mismatchfor all three, and-Werrorturns that into a linkfailure.
Every value stored in them is 0 or 1 -
N = P >> 7,Z = (P >> 1) & 1,C = P & 1, theSETZ/SETSZ/SETNCmacros, and the plainC = 0/C = 1assignments in
runtime.c- so this narrows the definition to match the headerrather than widening the header. That also matches how
A,X,YandSarealready declared and defined.
Verification
All three targets build clean from scratch,
measurecharacterises all 256opcodes, and deleting a binary triggers a relink.
cbmbasic --benchmarkoutputis byte-for-byte identical to a pristine build of master, with and without
-flto: 33155 half-cycles, same final CPU state.Notes
-march=nativeis slower on my machinemeasureis worse. Same the other way around.I used an LLM to help me out in this work, but I manually reviewed all changes made.