keirf / flashfloppy

Floppy drive emulator for Gotek hardware
Other
1.34k stars 193 forks source link

Startup hang with GCC 12 #660

Closed ejona86 closed 2 years ago

ejona86 commented 2 years ago

Still need to poke at it more, but GCC 12 seems to break at least at32f435. When building with GCC 12 flash floppy hangs when starting. It has hung in two different spots in the startup process, so I question whether it is actually a GCC 12 issue or whether GCC 12 just produced different enough output to expose a preexisting bug. For example, maybe some interrupt handling goes awry. I need to investigate further to see if other targets are impacted.

Both v3 and v4 were impacted. Building with GCC 11.3.0 instead of GCC 12.1.0 resolved the startup hang.

keirf commented 2 years ago

These are your own built toolchains I guess? The GitHub runner's still on GCC9 as am I :) No doubt there are plenty of UB gremlins in FlashFloppy that a newer gen compiler can sniff out...

keirf commented 2 years ago

Okay, I built my own toolchain. I have fixed some build errors as above. In testing I don't see any startup hangs at all! This is tested with both debug and prod builds of at32f435, tested as far as reading a few tracks from an Amiga ADF via Greaseweazle. This was a build of v3.33 (plus above patches).

Some further problems I do have:

  1. I had to specify -ffreestanding or I saw an error including stdint.h. Perhaps I didn't build my gcc correctly, but I'd really rather avoid -ffreestanding as it bloats the binary because gcc no longer relies on occasional callouts to Standard Library functions which FlashFloppy does in fact provide.
  2. Debug build of STM32F105 on master no longer fits in Flash. In fact it hasn't for ages, but it seems that the linker only checks that .text fits: There is no complaint when .data, which is relocated to SRAM but loaded immediately after .text, overflows Flash. Yikes! This may deserve another ticket, as the build really needs to be checking the binary fits in Flash. May need a script if we can't persuade the linker to do it correctly.
keirf commented 2 years ago

^ Above reworking of history avoids excessive growth of f_open() and is arguably clearer that it cannot possibly introduce any regression.

Bug in checking that binary fits in Flash remains. But the build passes! :)

keirf commented 2 years ago

I have fixed my issue (1) above. I needed to build GCC against newlib headers. My configure line (for my own reference):

../gcc-12.1.0/configure --prefix=/home/keir/arm-gcc/install --target=arm-none-eabi --enable-languages=c --disable-libssp --with-headers=../newlib/newlib/libc/include/
ejona86 commented 2 years ago

I'm using the Arch toolchain (its configure line). I removed -Wall temporarily (I actually fixed the warnings locally as well, but as I was doing lots of testing it was easier to ignore the warnings).

I did not need -ffreestanding, but my GCC is built with newlib.

I hadn't expected you to poke at this at all; I was making this more so I wrote down what I knew so I could come back to it later. Interesting you aren't seeing the hangs though.

Debug build of STM32F105 on master no longer fits in Flash. In fact it hasn't for ages, but it seems that the linker only checks that .text fits

Yikes indeed, although I've not noticed that before and I have exceeded flash size on multiple occasions. I'm using binutils 2.38 and it has seemed like the size checks were accurate.

The sizes seem to be fine for me (this is on master):

94908 out/at32f435/debug/floppy/target.bin
88716 out/at32f435/prod/floppy/target.bin
95984 out/stm32f105/debug/floppy/target.bin
89820 out/stm32f105/prod/floppy/target.bin
keirf commented 2 years ago

Huh, my sizes are different than yours:

commit a4314235c39380934152f8fa6759df61c167f961 (HEAD -> master, origin/master, origin/HEAD)
arm-none-eabi-gcc (GCC) 12.1.0
GNU ld (GNU Binutils) 2.38
95144 Jun 23 15:52 ./out/at32f435/debug/floppy/target.bin
88900 Jun 23 15:52 ./out/at32f435/prod/floppy/target.bin
96256 Jun 23 15:52 ./out/stm32f105/debug/floppy/target.bin
89996 Jun 23 15:52 ./out/stm32f105/prod/floppy/target.bin

Limit for stm32f105 main firmware is 96256 bytes so it's right on the limit for me with gcc-12.1.

You would be unlikely to see a problem with the debug build: You have 2kB leeway to the end of Flash, because that last erase block is used for cached FF.CFG data. So if you Flash a debug build and use it once, you'd never see a problem even if your USB stick has an FF.CFG, because writing that config to Flash will only blow away static data -- and that would only be realised if you reset into the same debug firmware again.

I wonder why your toolchain is producing such different binaries. It's too many bytes to be accounted for by src/build_info.c. I don't suppose it's that interesting really, but it maybe explains why my builds work and yours don't (even if it is ultimately a FlashFloppy bug that you're tripping over).

keirf commented 2 years ago

Here are the sizes produced by Ubuntu's gcc9 by the way (same as used by GitHub runners):

-rw-rw-r-- 1 keir keir 95468 Jun 23 16:06 ./out/at32f435/debug/floppy/target.bin
-rw-rw-r-- 1 keir keir 89184 Jun 23 16:06 ./out/at32f435/prod/floppy/target.bin
-rw-rw-r-- 1 keir keir 96616 Jun 23 16:06 ./out/stm32f105/debug/floppy/target.bin
-rw-rw-r-- 1 keir keir 90356 Jun 23 16:06 ./out/stm32f105/prod/floppy/target.bin

Note that the 105 debug build is too big, but the build didn't fail. Because _etext is within the 94kB limit:

0801f7f8 T _etext

I hadn't noticed this before either. Now I think about how the linker is accounting static data to the memory regions it knows about, it makes sense. I will fix it with a new build script in due course.

keirf commented 2 years ago

That's me done on this ticket. I did nobble the 105 debug build in master as it won't properly fit without stripping some more stuff out. Possibly it would be nicer to disable building that target altogether for now, but that makes the Makefile messier.

I did make my GCC12 configure line very close to Arch's but it didn't affect my object sizes at all. Perhaps Arch carries some patches on top of vanilla 12.1.0?

ejona86 commented 2 years ago

I reproduced the issue on at32f415+stable-v3 as well. It turned out to be a GCC optimization that replaced a loop with a call to strlen, but that loop was within strlen. The assembly was entertaining: https://godbolt.org/z/h383GeTav .

I don't know why you are getting different sizes, unless you forgot to clean. It seems the size should definitely change, although maybe not by much. Arch doesn't apply any patches (there'd be a call to patch within that build file).

keirf commented 2 years ago

Yes, no idea why I didn't reproduce this. Thanks for digging into it. I think your solution is indeed best, we don't need a blanket-applied option, let alone -fno-builtin.

keirf commented 2 years ago

By the way, VPATH sucks and we don't need it. The incremental mkdir instead of rsync is fine with me though, so I kept that.