Skip to content

Conversation

@WhenGryphonsFly
Copy link
Contributor

@WhenGryphonsFly WhenGryphonsFly commented Jun 6, 2023

Supersedes and closes #35.
Supersedes #53.

This PR fixes an issue where some games (e.g., Pokémon Pinball: Ruby and Sapphire) have nonmatching functions under agbcc. This is because these games were compiled with a compiler revision that occasionally pushed LR onto the stack when unnecessary, but otherwise appears to be identical to agbcc.

Unlike #35 and #53, this version of -fprologue-bugfix is defined for OLD_COMPILER/old_agbcc. This is because it is an error to pass an invalid flag to old_agbcc. This causes an issue in pokepinballrs where some files are compiled with agbcc and some are compiled with old_agbcc, but both use the same set of flags. (That is to say, it is easier to define it for OLD_COMPILER than it is to update the Makefile. Especially since I do not know how I would update the Makefile to accomplish this.)

As the Makefile of pokepinballrs has been updated, this is now a copy of #35. Please see below for more information. This has been left open due to the CI issues on #35, but it is functionally the same.

I can be contacted at WhenGryphonsFly#2089 on Discord.

@jiangzhengwenjz
Copy link

I don't see why this is better. With #35, when the flag is passed to old_agbcc, it's simply going to be ignored. And I believe pinballrs always uses the same compiler (the one patched by #35, which has agbcc codegen but the same push/pop pattern as old_agbcc)?

@WhenGryphonsFly
Copy link
Contributor Author

Pokepinballrs uses both compilers (the default compiler is agbcc, but some files such as gbplayer.c use old_agbcc.

Also, this is what happens if I use #35's toplev.c instead of this PR's version:
image
As can be seen, old_agbcc does not ignore the invalid option, it considers it an error.

@jiangzhengwenjz
Copy link

jiangzhengwenjz commented Jun 6, 2023

Pokepinballrs uses both compilers (the default compiler is agbcc, but some files such as gbplayer.c use old_agbcc.

This is probably a hack, because sometimes agbcc and old_agbcc have the same codegen (except for push/pop) and the guy needs old_agbcc to not generate the push/pop.

As can be seen, old_agbcc does not ignore the invalid option, it considers it an error.

Oh right. I somehow got it backwards when comparing your PR with #35, sorry.

@WhenGryphonsFly
Copy link
Contributor Author

This is probably a hack, because sometimes agbcc and old_agbcc have the same codegen (except for push/pop) and the guy needs old_agbcc to not generate the push/pop.

You may be correct. I checked all of the files using old_agbcc, and most compiled the same with agbcc. However, there were still two files that differ in code generation with old_agbcc. I will review them further.

@jiangzhengwenjz
Copy link

It's reasonable that some files are using old_agbcc. For example, agb_sram is part of the precompiled static library libagbbackup so it can be compiled with different compiler/flags than the source files written by the devs (the same applies to m4a). Basically they have no reason to use different compilers for their own source code...

@WhenGryphonsFly
Copy link
Contributor Author

Given that agb_sram.c and m4a.c were the two files remaining, I will then assume that yes, they both used old_agbcc. (I was suspecting as much; one function in agb_sram.c was giving very different codegen.)

Which leaves us back at the start; pokepinballrs uses both compilers, and I have very limited knowledge when it comes to Makefiles. I'll try working on the Makefile (with only two files I think I can work something out), but I'm going to leave this up for now. My understanding is #35 is stalled because the PR author never rebased to rerun the CI, and given that I have a pokepinballrs branch that depends on either #35 or this, I do have a vested interest in getting something merged in quickly.

@jiangzhengwenjz
Copy link

You can use target-specific variable values. Basically something like this: https://github.com/pret/pokepinballrs/blob/master/Makefile#L154
And you just need to set the flags for the m4a source.

@WhenGryphonsFly
Copy link
Contributor Author

Okay, I've updated my pokepinballrs branch. I'll add the #ifndef OLD_COMPILER back in, but as stated above I'll leave the PR open because of the CI issues with #35. Whichever of the two gets merged in doesn't matter to me.

@luckytyphlosion luckytyphlosion merged commit b5f1974 into pret:master Aug 13, 2023
@mid-kid
Copy link
Member

mid-kid commented Aug 13, 2023

@jiangzhengwenjz this is not a hack - there's a specific released version of the compiler, with the filename ThumbPatch_en.zip, that matches this exact behavior. It sits somewhere between old_agbcc and agbcc, and there's backup files in the source code that suggest its existence. See mid-kid/arm-000512@79d7d46

@jiangzhengwenjz
Copy link

@mid-kid You misread my reply. I was saying ppl's configuration in makefile was a hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants