dciabrin / ngdevkit

Open source development for Neo-Geo
GNU Lesser General Public License v3.0
272 stars 26 forks source link

Fix bss and data segments sometimes getting displaced (yes, again) #86

Closed KScl closed 1 year ago

KScl commented 1 year ago

So, while everything looked fine and dandy with the last commit to fix backup RAM when I ran it through the example suite, as soon as I started actually working on the project I've been working on again, I encountered a ton of things getting clobbered once more; moreover, things in the data section appeared to be getting initialized to 0 instead of their expected values. Debugging in MAME showed that the data section's contents were in fact sitting in memory, right where I expected them, but the code wasn't ever touching them afterwards and was writing somewhere else...

...an hour of pulling my hair out later and checking the symbols in the resulting ELF file with a fine toothed comb, the symbol for literally every variable that was defined in the bss and data sections were offset by exactly the size of the backup RAM section, while __bss_start_in_memory and __data_start_in_memory held the values I actually expected them to have.

Linkscripts are cursed.

dciabrin commented 1 year ago

Apologies for the late update,

Indeed, you're correct, I overlook that with the example suite :( When dumping the symbols generate by e.g. example 11-backup-ram, I can see that oddly, constant __bss_start_in_ram is being assigned the expected value (0x100008), but the start address of the .bss segment itself is not correctly configured:

 m68k-neogeo-elf-objdump -x rom.elf  | grep -v -e '\.bss\.bios' -e 'O .bss' | grep -e 'Idx Name' -e '\.bss' -e __bss_start_in_ram                  
Idx Name          Size      VMA       LMA       File off  Algn
  1 .bss.bram     00000008  00100000  00000570  00006000  2**1
  4 .bss          0000004e  00100000  00000570  00004000  2**2
00100000 l    d  .bss.bram  00000000 .bss.bram
00100000 l    d  .bss   00000000 .bss
00100008 g       .text.boot 00000000 __bss_start_in_ram

However, if I forcefully set this constant start address in the linkscript:

__bss_start_in_ram = 0x100008;

Now the offset seems to be correctly taken into account:

m68k-neogeo-elf-objdump -x rom.elf  | grep -v -e '\.bss\.bios' -e 'O .bss' | grep -e 'Idx Name' -e '\.bss' -e __bss_start_in_ram
Idx Name          Size      VMA       LMA       File off  Algn
  1 .bss.bram     00000008  00100000  00100000  00006000  2**1
  4 .bss          0000004e  00100008  00000570  00004008  2**2
00100000 l    d  .bss.bram  00000000 .bss.bram
00100008 l    d  .bss   00000000 .bss
00100008 g       .text.boot 00000000 __bss_start_in_ram

It seems to be that the original logics in the linkscript was not wrong, but I wrongly assumed lazy evaluation would work while it doesn't. Whether it's a semantics misunderstanding or a limitation in ld, I'd like to give it another try and fix that without losing the ability to manually override the backup ram location.

dciabrin commented 1 year ago

GitHub actions automatically closed this PR because I referenced it in commit d4077868bf9b5708db2af800d293560f7dd45935. When I run objdump on the example above, I can now see that .bss is correctly linked past the .bss.bram segment:

m68k-neogeo-elf-objdump -x rom.elf  | grep -v -e 'O .bss' | grep -e '\.data' -e 'Idx Name' -e '\.bss' |  head -6
Idx Name          Size      VMA       LMA       File off  Algn
  2 .bss.bram     00000008  00100000  00100000  00006000  2**1
  3 .bss.bios     0000020e  0010fcee  0010fcee  00007cee  2**1
  4 .bss          0000004e  00100008  00000560  00004008  2**2
  5 .data         0000006c  00100058  00000560  00004058  2**2
  6 .data2        00000000  001000c4  001000c4  00005f86  2**0

The commit also added C macros to fix the ability to override the address of the backup ram segment in RAM. For example, if I set the bram segment to start at 0x120000, I can see .bss is linked at 0x100000 as expected:

Idx Name          Size      VMA       LMA       File off  Algn
  2 .bss.bram     00000008  00102000  00102000  00006000  2**1
  3 .bss.bios     0000020e  0010fcee  0010fcee  00007cee  2**1
  4 .bss          0000004e  00100000  00000560  00004000  2**2
  5 .data         0000006c  00100050  00000560  00004050  2**2
  6 .data2        00000000  001000bc  001000bc  00005f86  2**0

The macros can also force the size of the .bss.bram segment to preallocate a large area. E.g. for a segment of 4KiB, the .bss segment starts right after the 4KiB in RAM:

  2 .bss.bram     00000008  00100000  00100000  00006000  2**1
  3 .bss.bios     0000020e  0010fcee  0010fcee  00007cee  2**1
  4 .bss          0000004e  00101000  00000560  00003000  2**2
  5 .data         0000006c  00101050  00000560  00003050  2**2
  6 .data2        00000000  001010bc  001010bc  00005f86  2**0

I think this commit should fix the issue you reported originally (thanks again for that), as well as fixing the ability to override the area of the backup ram (which I'm not sure worked fine prior to the linkscript refactoring).

Closing it now. Feel free to reopen if I think the fix is incomplete or doesn't work as advertised.