Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix __FILE__ macro #97

Merged
merged 4 commits into from
Oct 7, 2024
Merged

Fix __FILE__ macro #97

merged 4 commits into from
Oct 7, 2024

Conversation

JCog
Copy link
Collaborator

@JCog JCog commented Oct 1, 2024

So it turns out __FILE_NAME__ wasn't implemented until gcc-12, which isn't standard in Ubuntu until 24.04. The GitHub action won't be a problem once ubuntu-latest has finished its rollout to that version by the end of this month, but it's still a bit of an unreasonable requirement for users. Instead, this uses -fmacro-prefix-map to remove the path from __FILE__ at compile time. I also went ahead and applied it throughout the whole project, which will save a bit of space anywhere asserts are used, and as far as I can tell it isn't breaking anything.

Additionally, I added a runtime check at boot for effect sizes. Doing it at compile time will of course be better, but until that's implemented I thought I might as well add it.

And finally, python formatting was fixed so the Python Formatting & Linting action stops failing.

tools/build/configure.py Outdated Show resolved Hide resolved
src/effects.c Outdated Show resolved Hide resolved
@JCog JCog force-pushed the fix-filename-macro branch 2 times, most recently from 3c5abbd to 5c0f53f Compare October 2, 2024 18:34
@JCog
Copy link
Collaborator Author

JCog commented Oct 2, 2024

Okay, no longer messing with __FILE__ at all. Using a new ASSERT_FX macro like clover suggested that just passes the effect ID and line number, no file or function name. Asserts still don't get proper backtraces when using the TLB for some unrelated reason, but the screenshot shows what using the macro elsewhere looks like. Also made the suggested change for check_effect_sizes and clear_effect_data.

image

@JCog
Copy link
Collaborator Author

JCog commented Oct 2, 2024

Figured out why the asserts weren't working with the TLB! Turns out the shims have to be added to effectFuncs as well for the python script to see them, and is_debug_panic wasn't there.

image

@JCog
Copy link
Collaborator Author

JCog commented Oct 2, 2024

Okay, sorry about all the changes, but this is so much better now. It turns out saving everything but the message is redundant for all asserts if the logic is reworked slightly. The only slight downside is that now you need to compile with --debug to see more than the function name, but that's in line with regular crashes anyway. We do still need a special assert for effects since just the message is enough to put us over, but now the biggest effect is only 0xFE bytes, so there's actually a small amount of breathing room.

I also got backtraces working through virtual memory! And with that I'll stop adding things to this PR before it gets any more out of hand. 😅

image

@bates64
Copy link
Owner

bates64 commented Oct 3, 2024

Patchset looks good to me, but I'm getting this error at boot:

image

Compiler is mips-linux-gnu-gcc (GCC) 13.2.0

@JCog
Copy link
Collaborator Author

JCog commented Oct 3, 2024

Does it still do that if you clear ccache? I was having issues with that when I was messing with configure.py, though that's way bigger than I've ever had any effects, so very weird either way.

@bates64
Copy link
Owner

bates64 commented Oct 3, 2024

Nope, ccache -C didn't resolve it

@JCog
Copy link
Collaborator Author

JCog commented Oct 3, 2024

That is super strange. I tried running ccache -C and then ./configure --clean again and it still works, and I even tried using the bps generated by the GitHub action, which also works. My compiler is mips-linux-gnu-gcc (Ubuntu 12.3.0-17ubuntu1) 12.3.0, which is the only other thing I can think of that would make a difference, but yeah I can't reproduce this. Any chance you could send me a patch of your rom and the map file?

@bates64
Copy link
Owner

bates64 commented Oct 3, 2024

papermario-bad.zip

@JCog
Copy link
Collaborator Author

JCog commented Oct 6, 2024

Okay after parsing through the effect sizes between our two ROMs, it turns out that many of them are different sizes, and neither ROM always has the bigger or smaller one. Nothing jumps out at me when looking in a hex editor, so I'm inclined to think it really might just be compiler differences. In fact, energy_in_out is the only effect over 0x1000 bytes.

Instead of an assert, what if I just print a warning? And then later we can implement larger effects with consecutive TLB pages like clover suggested in discord to actually solve the problem.

@bates64
Copy link
Owner

bates64 commented Oct 7, 2024

In fact, energy_in_out is the only effect over 0x1000 bytes.

#98 successfully reports:

segment 'effect_energy_in_out' is oversized! (size is 1200; +200 compared to max size 1000)

Will merge this and remove the runtime check in 98.

@bates64 bates64 merged commit 04d65cd into bates64:main Oct 7, 2024
3 of 4 checks passed
@JCog JCog deleted the fix-filename-macro branch October 8, 2024 02:10
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.

2 participants