Modern GCC and GDB #624

dnasdw · 2024-06-09T14:44:49Z

Support compilation with modern GCC and enable source-level debugging using GDB (e.g., in VSCode with mGBA).

Eebit

These are some good changes and much appreciated to have some of these things cleaned up! I've left my preliminary thoughts about some of the fixes in the actual decompiled code.

For the changes related to the build system, I think it would be best to have them reviewed by @laqieer, @CT075, @StanHash, or @MokhaLeee (who each have a better grasp on the various aspects of it than me).

Eebit · 2024-06-09T21:37:39Z

src/bmmenu.c

@@ -1000,7 +1000,7 @@ void ItemSubMenuEnd(struct MenuProc* menu) {
    return;
 }

-u8 MenuCommand_SelectNo(struct MenuProc* menu, struct MenuItemProc* menuItem) {
+u8 MenuCommand_SelectNo(struct MenuProc* menu/*, struct MenuItemProc* menuItem*/) {


I think it would be good to annotate this with a further comment about why the second argument is commented out -- the comment at the latter call site (L1027-L1028) is now no longer obvious as to what it was pointing out

Eebit · 2024-06-09T21:39:56Z

src/bmmenu.c

    // This is really caused by implicit declaration
-    isBallista = ((s32 (*)(struct Trap*))IsBallista)(trap); // TODO: FIXME: UB
+    isBallista = IsBallista(trap);


Same here - I think it becomes unclear as to what this comment is indicating about the implicit declaration. Would be good to either remove the comment or make a comment about what the bug was, and how/why we fixed it

Eebit · 2024-06-09T21:42:37Z

src/bmunit.c

@@ -989,7 +989,7 @@ void UnitChangeFaction(struct Unit* unit, int faction) {
        GetUnit(newUnit->rescue)->rescue = newUnit->index;
 }

-inline s8 CanUnitCrossTerrain(struct Unit* unit, int terrain) {
+inline s32 CanUnitCrossTerrain(struct Unit* unit, int terrain) {


Is it at all possible to avoid this change? I'm a bit worried since it pollutes a lot of places in the code where we now have to cast to s8.

It seems pretty clear that this function is intended to return s8/bool, but there is just the one place (in mapanim_summon.c) where it returns s32 due to implicit declaration...

If it cannot be helped then it's okay, but if there is a less invasive approach I'd prefer we take that

This is likely another bug in the official code. Theoretically, there should only be a version of the function with a return type of s8. However, under the previous approach—where the function was not declared in the header file (.h) but individually declared in each source file (.c)—modern gcc would fail to link, unable to find the symbol for this function.

The current modification, while not ideal, ensures both the binary consistency after compilation with agbcc and resolves the linking issue with modern gcc.

CT075 · 2024-06-10T01:40:00Z

Makefile

 AS := $(PREFIX)as$(EXE)
 LD := $(PREFIX)ld$(EXE)
 OBJCOPY := $(PREFIX)objcopy$(EXE)
 STRIP := $(PREFIX)strip$(EXE)

-CC1     := tools/agbcc/bin/agbcc$(EXE)
-CC1_OLD := tools/agbcc/bin/old_agbcc$(EXE)
+# note: the makefile must be set up so MODERNCC is never called


Can you give more details on what this is accomplishing?

Mainly refer to https://github.com/pret/pokeemerald

What parts of that repo is this intended to imitate? Should I believe that this is a good thing solely because emerald does it?

This change is mainly inspired by the Makefile section, which allows for the compilation of different versions of the code. Intermediate files and .o files are generated in separate directories to ensure that switching between different versions during compilation does not cause interference.

laqieer · 2024-06-10T03:12:28Z

Thanks for your contribution. Could you please provide these to help us review this PR:

resolve the conflicts with target branch first
pipeline result to prove it passes and breaks nothing
information on your testing environment (platform, OS, toolchain)
result of make command to prove it builds with agbcc and matches the vanilla ROM
result of command to build with modern gcc such as make modern and prove it works on hardware/emulator
result of command to build with debug info such as make modern DINFO=1 and proof of source-level debugging (also gdb version and emulator version)
add related instructions to README

Fix -Wimplicit-function-declaration Fix -Werror=builtin-declaration-mismatch

Fix MenuCommand_SelectNo

…return-mismatch]

Fix IsBallista

…ror=switch-outside-range]

…re incorrectly compiled into the .bss segment and discarded in the latest GCC version.

… using GDB (e.g., in VSCode with mGBA).

dnasdw · 2024-06-10T07:04:48Z

I performed a rebase and then made corresponding additions in ldscript_modern.txt based on the changes in ldscript.txt today. I removed two inappropriate comments and added two new comments elsewhere to explain why the changes were made.

There are no conflicts now, and I manually compiled both versions without any issues.
The testing environment includes MSYS2, WSL1 (Ubuntu-22.04), and the operating system is Windows11, with the toolchain being devkitARM.
To compile the modern version of gcc, you can use either make modern or make MODERN=1.
There's no need for parameters like DINFO=1 because the project's compilation parameters already include -g, so the compiled code is debuggable. However, when the -O2 optimization is enabled, the game freezes during the map and story background description, and removing -O2 solves the issue. Even pressing the START button to skip also leads to freezing, likely due to some animations.

As for README and similar files, it's best for your team to write them as I'm not particularly skilled in handling such textual content.

dnasdw · 2024-06-10T07:07:21Z

Thanks for your contribution. Could you please provide these to help us review this PR:

resolve the conflicts with target branch first

pipeline result to prove it passes and breaks nothing

information on your testing environment (platform, OS, toolchain)

result of make command to prove it builds with agbcc and matches the vanilla ROM

result of command to build with modern gcc such as make modern and prove it works on hardware/emulator

result of command to build with debug info such as make modern DINFO=1 and proof of source-level debugging (also gdb version and emulator version)

add related instructions to README

Forgot to quote this, I mentioned the relevant content in my reply above.

Eebit reviewed Jun 9, 2024

View reviewed changes

CT075 reviewed Jun 10, 2024

View reviewed changes

dnasdw added 20 commits June 10, 2024 13:12

Fix -Werror=shift-overflow

cc06f7b

Fix error: lvalue required as left operand of assignment

066c925

Fix -Wincompatible-pointer-types

e2433bd

Fix error: conflicting type qualifiers for ...

9e584c2

Fix error: array type has incomplete element type 'struct REDA'

ac314fb

Fix various warnings and errors caused by missing #include <string.h>.

32b81f2

Fix -Wimplicit-function-declaration Fix -Werror=builtin-declaration-mismatch

Fix error: ... causes a section type conflict with ...

8deca19

Fix -Werror=aggressive-loop-optimizations

cb24751

Fix error: function called through a non-compatible type [-Werror]

380bae1

Fix MenuCommand_SelectNo

Fix error: 'return' with no value, in function returning non-void [-W…

4ec800d

…return-mismatch]

Fix error: function called through a non-compatible type [-Werror]

a24c5ae

Fix IsBallista

Fix error: conflicting types for ...

1786284

Fix -Werror=overflow

6a4215a

Fix -Werror=stringop-overread

483332a

Fix error: case label value is less than minimum value for type [-Wer…

da4e6d3

…ror=switch-outside-range]

Fix multiple definition of ...

c26ebd4

Fix undefined reference to ...

74787f8

Fix the issue where zero-initialized variables in non-.bss segments a…

430d14d

…re incorrectly compiled into the .bss segment and discarded in the latest GCC version.

Support compilation with modern GCC and enable source-level debugging…

d679902

… using GDB (e.g., in VSCode with mGBA).

Updated the comments in the code for better clarity and accuracy.

1a7cd7e

dnasdw force-pushed the gdb branch from 6222205 to 1a7cd7e Compare June 10, 2024 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modern GCC and GDB #624

Modern GCC and GDB #624

dnasdw commented Jun 9, 2024

Eebit left a comment

Eebit Jun 9, 2024

Eebit Jun 9, 2024

Eebit Jun 9, 2024

dnasdw Jun 10, 2024

CT075 Jun 10, 2024

dnasdw Jun 10, 2024

CT075 Jun 10, 2024

dnasdw Jun 10, 2024

laqieer commented Jun 10, 2024

dnasdw commented Jun 10, 2024

dnasdw commented Jun 10, 2024

Modern GCC and GDB #624

Are you sure you want to change the base?

Modern GCC and GDB #624

Conversation

dnasdw commented Jun 9, 2024

Eebit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laqieer commented Jun 10, 2024

dnasdw commented Jun 10, 2024

dnasdw commented Jun 10, 2024