search

Stephane-D / SGDK

SGDK - A free and open development kit for the Sega Mega Drive
https://www.patreon.com/SGDK
MIT License
1.7k stars 179 forks source link

ROM larger than it should be, links unneeded symbols from libmd.a #317

Closed joeyparrish closed 3 months ago

joeyparrish commented 3 months ago

I built a ROM with SGDK that had very little in it, and it was 512kB. When I looked at symbol sizes with nm --print-size --size-sort --radix=d out/rom.out, I found that these three symbols were 128kB each:

00046904 00131072 t log10tab_f16
00177976 00131072 t log2tab_f16
00315192 00131072 t sqrttab_f16

This is in spite of the fact that I'm not using log or sqrt or any fix16 functions at all. In fact, the methods that use those tables were not present in the ROM.

I spent some time playing around with and researching the compiler settings, and I eventually found this in the GCC optimization docs (emphasis added):

-funit-at-a-time

This option is left for compatibility reasons. -funit-at-a-time has no effect, while -fno-unit-at-a-time implies -fno-toplevel-reorder and -fno-section-anchors.

Then this (emphasis added):

-fno-toplevel-reorder

Do not reorder top-level functions, variables, and asm statements. Output them in the same order that they appear in the input file. When this option is used, unreferenced static variables are not removed. This option is intended to support existing code that relies on a particular ordering. For new code, it is better to use attributes when possible.

When I experimented with the flags in the makefile, I found that removing -fno-unit-at-a-time fixed my issue. My ROM went from 512kB to 128kB.

Do you see any risk in removing this flag? If not, I will send a PR.

Thanks!

Stephane-D commented 3 months ago

Nice find ! Since sometime now I observed that randomly LTO wasn't properly eliminating some unused symbols, glad to see you probably find the source of that issue. The problem is that i do think that the -fno-unit-at-a-time option maybe useful somehow but i need to test it to ensure that. If it turns out that it's not anymore useful then we can indeed remove it !

Stephane-D commented 3 months ago

It turns out that -fno-unit-at-a-time was just used as an optimization flag so it's probably safe to remove it :) I already pushed a change.

joeyparrish commented 3 months ago

Wonderful, thanks! I will send a PR, but I will wait until we have settled the Docker changes.

Stephane-D commented 3 months ago

Should be fixed hopefully with the remove of -fno-unit-at-a-time flag.