pspdev / psptoolchain

A script to automatically build an open-source toolchain for PSP homebrew development.
378 stars 120 forks source link

Double-precision floating point is broken #85

Closed csnover closed 6 years ago

csnover commented 6 years ago

Compiling and running this trivial program causes a crash (PPSSPP claims it attempts to read an invalid address, and it crashes on real hardware too):

#include <pspkernel.h>
PSP_MODULE_INFO("template", 0, 1, 1);
PSP_MAIN_THREAD_ATTR(THREAD_ATTR_USER | THREAD_ATTR_VFPU);
int main(int argc, char *argv[]) {
    return (double)argc + 1.0;
}

(The only reason for argc use in this sample is to keep the compiler optimising away the operation.)

This program is just a modification of the elf_template SDK sample, so can be compiled in the usual manner for that sample (make -f Makefile.sample) with an appropriate param.sfo.

This test case runs without crashing using a compiler from commit 820386024b06bb31b72ae95209a835a9f6c37a51, but not with one from 665124fc2820ac532dc06e38b8118fd35e713537, so unless someone sneaked in a change to newlib when the URL changed, it looks like the update to GCC 4.9.3 broke this.

The use of double-precision floating point causes the compiler to switch to the software FP library, which is apparently not working correctly. Single-precision FP, which uses hardware, is fine. The generated assembly is similar to that generated by earlier versions of the compiler, with some different ordering, but I am not familiar with MIPS or PSP at all so it is unclear to me whether the problem is related to this reordering, is a problem with the runtime library, or what.

csnover commented 6 years ago

I guess this could be a duplicate of #65, or at least probably has the same root cause.

wjp commented 6 years ago

While investigating this, I found the symptoms of this are identical to those discussed at https://patchwork.ozlabs.org/patch/359867/ , with MIPS' -msingle-float ignored by libgcc.

The patch there was limited to the r5900 arch, but I've adapted it as suggested in that thread to hook into the __mips_single_float preprocessor define, so that it also gets used for the mipsallegrex architecture: https://www.usecode.org/scummvm/0001-Attempt-to-handle-__mips_single_float-in-libgcc.patch

Adding that patch to psptoolchain's gcc-4.9.3-PSP.patch yields 24678280484d9c61d5701017bf2d4c66b016a84f .

We (at the ScummVM project) have tested this with PPSSPP and a real PSP, and a brief test shows that this fixes the immediate problem, and disassembly of libgcc.a shows that the functions crashing before now have proper implementations.

However, the actual changes here to libgcc's target config as taken from the patch at https://patchwork.ozlabs.org/patch/359867/ are beyond my knowledge to review properly.

davidgfnet commented 6 years ago

I'm so sorry I broke the toolchain. Should we just revert the 4.9 gcc patch and use the 4.8.5 one? I think that one should not be affected. I see that the FP modes were reworked on 2014, which was the year that 4.9 was released. In particular we see it in here:

https://github.com/gcc-mirror/gcc/commits/e11be3ea01eaf8acd8cd86d3f9c427621b64e6b4/libgcc/config/mips/t-mips

My hypothesis, correct me please if I'm wrong, is that PSP doesn't support hardware double precision and 4.8 and before would use softfp for it. Now 4.9 seems to go for SF DF hardware always (except for some targets, to me it seems the test only checks whether the HW supports hardware FP, does not check single vs double), so whenever using DF it raises a CPU exception.

So bottom line: can someone correct me when I say the PSP does not support double precision on hardware? That seems to be the problem with the R5900 too, which got affected by this. To fix this we would need to patch it to enable a mixed mode with hard sf and soft df, which I imagine is possible since it was working before anway. Should we also promote to use gcc 4.8.5 in the meantime?

Oh also, now that I saw the patch at the bottom of that link, yeah that confirms my thoughts, we need a t-hardfp-sf that only enables hardfp for sf and not df. csnover, I can send an updated gcc 4.9 patch, would you have the time to test it in that case? Thanks and sorry for breaking your project!

csnover commented 6 years ago

I am with @wjp at ScummVM project. Using his patch for gcc 4.9 creates binaries that work correctly.

davidgfnet commented 6 years ago

Sweet! Then I'll merge it in the gcc 4.9 patch and pull request it. Maybe it's time for another gcc rebasing? :D