oot icon indicating copy to clipboard operation
oot copied to clipboard

Match retail BSS ordering in boot and code

Open cadmic opened this issue 1 year ago • 2 comments

Strategies used:

  • Declaring some variables in header vs only in the .c file. In -g3 it seems like the block number is assigned the first time a variable is declared. For these I renamed them from s to g (although none of them were technically static before and I'm not sure if that matters).
  • Adding dummy structs, using the new macros INCREMENT_BLOCK_NUMBER_BY_1(), INCREMENT_BLOCK_NUMBER_BY_10(), and INCREMENT_BLOCK_NUMBER_BY_100()
  • Moving variables and struct definitions around
  • Adding or removing scopes inside functions by moving local variables around

For some files I left a comment to hopefully make it easier to fix them if headers change. It's hard to do in general though, especially for files which have variables declared both in a header and in the .c file. It wasn't too bad to fix though with the new tooling and a spreadsheet so hopefully it's not too much harder to reorganize headers in the future.

Notes for each file:

  • idle.c: I changed gMainThread to sMainThread because it seems it should not be declared in a header, and adjusted block numbers.
  • z_actor.c: It could maybe work to move sCurCeilingPoly and sCurCeilingBgId later in the file, but this caused BSS ordering problems for debug. Instead, I added some block number padding in the middle.
  • z_camera.c: I had to add a large amount of padding between two in-function statics so I'm really unsure of how this was written originally. Padding in other places would also work.
  • z_collision_check.c: Without the sBssDummy variables added for debug, the block numbers actually line up very nicely. I removed some scopes and moved a struct definition to make room.
  • z_common_data.c: Added padding
  • z_demo.c: Here it does seem like D_8015FCCC, D_8015FCD0, and D_8015FCE4 should be global, and that the variables are declared in a different order than they're defined (maybe other ways are possible but it seems like it would require lots of padding)
  • z_kankyo.c: This is definitely the jankiest file here. First, it seems like gLensFlareUnused should not be declared in a header, so I renamed it to sLensFlareUnused. Second, it seems like gLightningStrike should be declared before the other variables, but with some space in between before the other z_kankyo variables. I couldn't find a clean way to do this so I moved sLightningFlashAlpha, sSunDepthTestX, and sSunDepthTestY to the header too but left a TODO for later reorganization. Finally, I added a large amount of padding before sNGameOverLightNode. It could be moved farther down in the file, but this causes BSS ordering issues for debug.
  • z_kaleido_scope_call.c: It seems like D_8016139C should be a global variable here.
  • z_play.c: It seems like sPlayVisMono must be global here, and it would be easier if sTransitionTile was too. I think it makes sense given the similarity to gVisMonoColor and gTransitionTileState respectively
  • main.c: Seems to require declaring variables in a different order in the header, and I moved gPadMgr to be closer to the others instead of in padmgr.h.
  • speed_meter.c: It seems like D_8016A578 should be a global variable here.
  • sys_math3d.c: Very fragile, requires a specific starting block number.
  • fault.c: The current placement of gFaultMgr makes this rather tight (it has block number 4 and the other variables must go before it), so I had to add different padding for retail and debug. This should be more lenient if the headers change.
  • fault_drawer.c: Required no changes and should not be fragile (has 1/256 chance of getting the wrong ordering)

After #1917, #1918, #1921, and #1926, gc-eu-mq is now completely matched up to the end of code (other than the ROM checksum).

cadmic avatar Mar 17 '24 18:03 cadmic

For some files it's the only way, for others it's not. In a little bit I'll write more about which files need what

cadmic avatar Mar 17 '24 20:03 cadmic

I reverted many of the s->g changes and added notes for each file in the PR description. I initially tried to put as many things in a header as possible because I thought that would be more robust to header changes, but I guess cleaner documentation is more important.

cadmic avatar Mar 18 '24 00:03 cadmic

Since this hasn't gotten much feedback yet I pushed BSS ordering fixes for overlays too. Actually the main difficulty there is that we don't have many options for controlling the ordering without -g3 which forces us to unnaturally declare variables at the top instead of spreading them out in the file. Spreading them out would make it easier to match the ordering in retail without tons of padding.

cadmic avatar Mar 21 '24 20:03 cadmic

I merged main and this PR now builds OK

cadmic avatar Mar 31 '24 05:03 cadmic

From discord: fig thinks the INCREMENT_BLOCK_NUMBER_BY macros are too noisy, maybe a #pragma and some kind of source preprocessor would be better

cadmic avatar Apr 07 '24 00:04 cadmic

I introduced tools/preprocess.py which will process #pragma increment_block_number N for all files in src. I made it do the reencoding too because it's already juggling temporary files and changing the encoding is very easy in Python.

This does slow the build down somewhat (by 10-20s with -j8) but maybe it's okay because that's basically how it was with asm-processor

cadmic avatar Apr 10 '24 17:04 cadmic

Thanks for being flexible and trying out a new approach, appreciate it. I like the way this looks alot more

fig02 avatar Apr 10 '24 17:04 fig02