Fix on fedora - #318
Conversation
generated bk.o2r carried no portVersion record. At runtime
ReadPortVersionFromOTR() returns {0,0,0} for an archive with no portVersion,
VerifyArchiveVersion() then compares that against the build version (1.0.0)
and fails, so the game reports "Outdated/incompatible ROM archives" and
drops into the re-extract flow on every launch.
Pass `-u ${PROJECT_VERSION}` to the torch o2r command (mirroring how
GeneratePortO2R already stamps lighthouse.o2r) so bk.o2r records a matching
portVersion and the version check passes.
…arbage thread5's event-vs-task discriminator read the full 8-byte OSMesg pointer (`(uintptr_t)msg.ptr < 100`). Hardware events are posted with OS_MESG_32(code), which sets .data32 but leaves the union's high 4 bytes uninitialized on a 64-bit host. When those garbage bytes came up non-zero (observed: the SP event arriving as 0x00007FCE_00000006), the event was mis-read as a task pointer and silently dropped. The dropped SP events were the graphics-completion SP and the yield-SP, so thread5_handleSPEvent never ran: a graphics task that had yielded (so an audio task could run) was never resumed, sUnkFlag1 stuck at TASK_YIELDED, the frame's DP -> sMesgQueue2 signal never fired, and the game hung right after ROM import. It was layout/timing-dependent because it rode on whatever stack garbage landed in those high bytes. Discriminate on msg.data32 (the event code, 3-13) instead of the full pointer. Task submissions carry a real pointer whose low 32 bits are always large, so they still route to the task branch. handleSPEvent now runs and the freeze is gone. Follow-up (libultraship): OS_MESG_8/16/32 should zero the whole 8-byte OSMesg union, not just the active member, so no other consumer can be bitten by the same uninitialized high bytes. This discriminator change is a defensive fix at the call site; fixing the macro removes the UB at its source.
|
Thanks for the PR! Do you mind removing or trimming the comments to one line? We try to stay away from big blocky comments. |
|
Of course! Thank you for this project! I pushed a new commit to the branch this morning to make the comments single line. |
|
Please also target the |
|
I can replay the changes on that branch and make a new pull request, would you prefer that? |
|
You should be able to edit your PR title and it will allow you to change the branch. |
|
Thank you, done |
JeodC
left a comment
JeodC
left a comment
There was a problem hiding this comment.
First, good shouts on the union hazard and the o2r stamp is good.
On graphics_thread.c, OSMesg is an 8-byte union and OS_MESG_32(x) writes only the low 4 bytes. Per C11 6.2.6.1p7, the bytes that don't correspond to the written member take unspecified values, so msg.ptr's upper half isn't guaranteed zero on any 64-bit build. Every consumer that discriminates on .ptr inherits that. A one-line change upstream fixes all of them at once:
#define OS_MESG_32(x) ((OSMesg){ .ptr = (void*)(uintptr_t)(x) })
You could open an upstream pull request to Kenix3/libultraship and all ports will benefit when they next bump after that merge. Lighthouse currently substitutes our own OS_Mesg, so the change is good to keep here as well.
That said, the same guard exists in one more place and this PR doesn't cover it. src/port/Game.cpp:100: if (!OnGameThread() || (uintptr_t)taskData < 100) { return; }
Under the same premise, that guard can pass with a garbage pointer and then dereference task->task_type. If anything it's the nastier of the two sites, since it's a crash rather than a dropped event. Could you add in a .data32 check there as well?
| ThreadWatchdog_Beat(WATCHDOG_THREAD5); // [port] one beat per serviced message | ||
| thread5_checkAndExecutePreNMI(); | ||
| if ((uintptr_t)msg.ptr < 100) { | ||
| // [port] OSMesg is an 8-byte union and OS_MESG_32 leaves its high bytes garbage, so discriminate on .data32 (the event code), not the full .ptr, which mis-read events as task pointers and dropped SP events (the post-import freeze). |
There was a problem hiding this comment.
Could you wrap this to the file's ~120 col style, and trim it to the mechanism?
| ```sh | ||
| # using gcc | ||
| dnf install gcc gcc-c++ git cmake ninja-build lsb_release SDL2-devel libpng-devel libzip-devel libzip-tools nlohmann-json-devel tinyxml2-devel spdlog-devel boost-devel libogg-devel libvorbis-devel | ||
| dnf install gcc gcc-c++ git cmake ninja-build lsb_release SDL2-devel SDL2_net SDL2_net-devel libpng-devel libzip-devel libzip-tools nlohmann-json-devel tinyxml2-devel spdlog-devel boost-devel libogg-devel libvorbis-devel |
There was a problem hiding this comment.
The clang variant needs these as well. I believe SDL2_net-devel pulls the runtime lib itself, so you can drop the bare SDL2_net from both lines if you'd rather keep them short.
2 participants
Fixes #317