Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARMJIT bug: One Piece Romance Dawn Bouken no Yoake [luffy stuck at 2 floor of morgan base] #2278

Closed
domschek opened this issue Jun 15, 2013 · 48 comments · Fixed by #6444
Closed
Assignees

Comments

@domschek
Copy link

On android (build 1345) luffy stuck at 2 floor of morgan base

it looks like when it is running normally:

http://www.youtube.com/watch?v=9sQLWcjnBOc [ Play the video from 4:18 min] [ LINK BROKEN ]

on android it's like as if he was running against a wall.

please fix or patched the problem please

sorry for my bad english im from germany

@unknownbrackets
Copy link
Collaborator

This only happens on Android? Did it start in a particular version, or has it always done this?

-[Unknown]

@domschek
Copy link
Author

I tried it on windows and it works without problems, I have tried it with the current version for android and at each
he runs against a wall.

I suspect that the ppsspp can not handle the "auto mode" in android

@unknownbrackets
Copy link
Collaborator

You might have to use savestates to answer this question with how slow it'll be, but does it work on Android using interpreter?

-[Unknown]

@domschek
Copy link
Author

did it with an original version and a patch version veruscht both do not go to the point unfortunately.

it is probably because of the emulator the video mode where the cahrakter
alone is directed can not be processed under android

I have a video of it filmed the times you see what I mean.
I will upload the video on youtube or adden in the forum?

sorry for the quality of the video better but unfortunately we went there not the apps
the record did not seem to work properly on record

@domschek
Copy link
Author

here is the link to the video where I filmed the times the problem:

http://www.youtube.com/watch?v=h6gsfdbesmo&feature=youtu.be

@unknownbrackets
Copy link
Collaborator

Ah, I see. This really looks like a CPU bug of some kind, I think...

I'm not sure I understood your previous comment - if you turn jit off, does it do the same thing? Or it won't even load the savestate?

-[Unknown]

@brujo5
Copy link

brujo5 commented Jun 15, 2013

@domschek

upload your save state.

whats build version using?

i can make a log file for unknownbrackets

@domschek
Copy link
Author

@brujo5

should I make a savestate verge or, when the error occurs?

using v0.7.6-1383-g856fe3e for android

@domschek
Copy link
Author

thanks for your help you guys are the best

@domschek
Copy link
Author

@unknownbrackets

when I turn off jit not even load the game and when
I it turn off the game it's the same problem

@domschek
Copy link
Author

@brujo5

i have a problem i cant uploadet the file

Error :

"Yowza, that's a big file. Please submit an image file smaller than 5MB."

@brujo5
Copy link

brujo5 commented Jun 15, 2013

@domschek

upload a external server like sendspace.

@domschek
Copy link
Author

Here the save state just before the error:

http://www.sendspace.com/file/88t1x9

@brujo5
Copy link

brujo5 commented Jun 15, 2013

@domschek

thx,i try.

@domschek
Copy link
Author

Here the savestate with the "error" :

http://www.sendspace.com/file/ychw4n

@domschek
Copy link
Author

no, I have to thx :-) thx for your good work guys

@brujo5
Copy link

brujo5 commented Jun 15, 2013

@unknownbrackets

log file whit jit enable

http://www.sendspace.com/file/9sbebt

on interpreter mode working but slow as hell

@brujo5
Copy link

brujo5 commented Jun 15, 2013

@unknownbrackets

here log interpreter mode [game working]

http://www.sendspace.com/file/mdk69n

@domschek
Copy link
Author

here is a video as it would have to run really, but I think that the ppsspp
the auto mode where the character is not automatically under android
can be processed :

http://www.sendspace.com/file/4qj1eu

@unknownbrackets
Copy link
Collaborator

So, that means this is very likely an armjit bug...

-[Unknown]

@domschek
Copy link
Author

Okay and can you fix the bug?

@domschek
Copy link
Author

@unknownbrackets

you can fix the bug at all? or is there a possibility to load the save states from andorid on windows and windows on android? because up to now only an error message always comes

@unknownbrackets
Copy link
Collaborator

Unfortunately, armjit bugs are hard and I only first saw this ~2 days ago. There are other people looking at armjit too, but it's hard to find these bugs.

Making savestates transfer between Android/Windows may be possible, but that'll also be kinda hard to debug. Do saves not work in this game?

-[Unknown]

@domschek
Copy link
Author

Oh okay then, of course Is. Since we can do nothing.

In the game "Normal" Save unfortunately is not because the ppsspp crashes when you go to save / load, and you save so only savestate and load loadstate :-(

@xsacha
Copy link
Collaborator

xsacha commented Jun 17, 2013

I'll have a look at this one. I'm eager to remove armjit bugs.
Thanks.

@ghost ghost assigned xsacha Jun 17, 2013
@domschek
Copy link
Author

@xsacha
Really? That would be great :-) Thanks for your help. Since your really the best

@xsacha
Copy link
Collaborator

xsacha commented Jun 19, 2013

I can't load that save state.

Can you confirm that rotating the camera (using L and R buttons) fixes the issue?
It seems to be a problem with the camera calculation.

That is, you always have to be looking either North or South. If you look directly east or west you can not move?

Also, I get a lot of crashes in this game. Is that normal?

Can't even get my own save states to work :( So not sure I can fix this.

OK, for me it keeps crashing in FFMPEG :\ I might try running it without ffmpeg on just so I can fix this armjit bug.

@domschek
Copy link
Author

Invite you to savestate under windows? or android?

under windows it runs so good so far the error appears dependent only on-android

I can be happy once again make a savestate for android?

With L and R button I wanted to demonstrate what I mean once the character of how he would run against a wall, by pressing the L or R button the error is not corrected.

the game crashes only when you load / save game goes on, so you can save the game only save and load state and load.

Although I do not know what it is exactly but I suspect it is because the
the ppsspp not the "auto" mode camera can not handle, since from there a while the character is controlled automatically. and you have to press buttons to make certain only the input.

Here is a video link as it is when it is running normally it shows what I mean by that is automatically controlled by the input mode and the character of the :

http://www.youtube.com/watch?v=1AjwQebUfik&feature=youtu.be

I hope I was able to help a little

@xsacha
Copy link
Collaborator

xsacha commented Oct 14, 2013

@domschek Are you still getting this error?
I fixed a rounding bug in armjit today.
Please advise.

@Stormrager
Copy link

nope, luffy is still endlessly running.

@unknownbrackets
Copy link
Collaborator

Savestates may now be compatible between Windows and Android.

Does anyone have this game and a compiler, and not crashes when playing this game?

If yes, it'd really help to know which function of the jit crashes it. Luckily, it's extremely easy (if a tiny bit time consuming, but really not that bad) to find it - as long as you have the game.

In Core/MIPS/ARM/ there are 4 relevant files:
ArmCompALU.cpp
ArmCompFPU.cpp
ArmCompLoadStore.cpp
ArmCompVFPU.cpp

In each file, there's two lines like this:

// #define CONDITIONAL_DISABLE { Comp_Generic(op); return; }
#define CONDITIONAL_DISABLE ;

Switch them in all 4 files to:

#define CONDITIONAL_DISABLE { Comp_Generic(op); return; }
// #define CONDITIONAL_DISABLE ;

(the VFPU one is a little different, but just switch the // part like above.)

Then, compile that, and the next thing is to verify your testing process. Try to load a savestate where he's already stuck. If he gets unstuck with the above - great, that's the savestate to use. If he's still stuck, try using the interpreter and loading the same state.

If the above changes don't work (and you're sure you updated your phone properly) and the interpreter does work, then stop here and reply to this issue with that. It's not very likely, but that means it's a whole different kind of issue.

If interpreter doesn't work either, you have to find a place where you can load and get the issue to not happen with the above changes.

Once you've got a starting point for testing, now it's time to narrow it down. Once you zero in on the exact problem here, it will definitely be a lot easier to fix than now (it still might be challenging, but significantly less challenging than it is without this testing.)

The first thing to do is to UNDO your change to 2 of the 4 files. I recommend undoing the change in ArmCompALU.cpp and ArmCompLoadStore.cpp. Then try again. This will make the speed a lot less horrible, and those two are probably not broken (I hope.)

If it does work, that means you know the problem is in ArmCompFPU.cpp or ArmCompVFPU.cpp. If it DOES NOT work, then you know it is in ArmCompALU.cpp or ArmCompLoadStore.cpp. Either way, you've cut the possible places the problem could be in half.

Now, take the remaining two files, and do the same thing - so now, you'll have the original code in 3 files, and the change from above in only one file. This'll tell you which file of the 4 it is in.

Once you know which file, the next thing is to narrow it down to the specific function. Again, we're going to do this by halves since it's the fastest way. First, undo all your changes so you're back to the original code in all four files. Then, go half way down the file, and find a line that looks similar to this:

void Jit::Comp_VScl(MIPSOpcode op) {

Right before it, add this:

#undef CONDITIONAL_DISABLE
#define CONDITIONAL_DISABLE { fpr.ReleaseSpillLocks(); Comp_Generic(op); return; }

Now try again. If the game works, the problem function is somewhere below that line. If it doesn't, it's above. You can continue moving those lines by halves until you've narrowed it all the way down to one function. It won't take that many tries, probably less than 7.

At that point, we'll know what function is causing it. In the worst case, we can make all games slightly slower by disabling that one function (making it use the interpreter instead.) You can do this yourself by finding the CONDITIONAL_DISABLE inside that function, and changing it to simply DISABLE.

It will also tell us where to concentrate our testing.

-[Unknown]

@phiupp
Copy link

phiupp commented Jan 10, 2014

Where do I find these 4 relevant files?? Do I find them on my smartphone??

@unknownbrackets
Copy link
Collaborator

@phiupp they are part of PPSSPP's source code. You need a compiler (which is free and easy to get) to try this change. Until someone who has the game can try it, we're pretty stuck.

-[Unknown]

@thedax
Copy link
Collaborator

thedax commented May 5, 2014

Out of curiosity, can someone with this game test if v0.9.8-594-gfe696dc or newer helps with this issue? Unknown and I fixed a couple bugs in the ARM JIT last night.

@thedax
Copy link
Collaborator

thedax commented May 5, 2014

Well, according to obi55 on the forum, the flags fixes didn't help this game. Darn.

@hrydgard
Copy link
Owner

hrydgard commented May 5, 2014

Well, unknown's guide to debugging armjit issues above still applies, so someone with the game will just have to do it...

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

Confirmed this bug still exists on Android, Blackberry, Symbian.
I'm assigned to this so I'll try to fix it right now.

Looks like it's in FPU.

Edit: Problem function is Comp_FPU2op
case 15:
...
MOVI2F(S0, 0.4999999f, SCRATCHREG1);
...

In trying to replicate: FsI(fd) = (int)floorf(F(fs)); break; //floor.w.s

@unknownbrackets
Copy link
Collaborator

Awesome. This is one implementation of floor (neon but similar):
https://code.google.com/p/math-neon/source/browse/trunk/math_floorf.c?r=18

It doesn't look too bad for ceil/floor...

-[Unknown]

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

Doesn't look like that needs NEON? Just a VCMP instead of VGT.
So basically convert to an integer and back to a float. Then subtract that value by 1 if it's greater than the original value.
Too bad VCVT is so slow although I guess it is better than interpreter.

@unknownbrackets
Copy link
Collaborator

Updating the rounding mode is probably slow, right? We could also try caching the FPCSR rounding mode, if we hope the game uses the same rounding types in a row. Not sure what would be fastest..

-[Unknown]

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

Is it possible that there's a problem with NaN's instead?

I tried this new method for floorf but it isn't working either.

    fpr.MapDirtyIn(fd, fs);
    VCVT(fpr.R(fd), fpr.R(fs),        TO_INT | IS_SIGNED);
    VCVT(S0, fpr.R(fd),               TO_FLOAT | IS_SIGNED);
    VCMP(S0, fpr.R(fs));
    SetCC(CC_GT);
    MOVI2F(S1, 1.0f, SCRATCHREG1);
    VSUB(S0, S0, S1);
    VCVT(fpr.R(fd), S0,               TO_INT | IS_SIGNED);
    SetCC(CC_AL);

I even tried adjusting the original case 15 to be higher/lower and that didn't change it.
Setting it to just round() as in case 12, however, did work (for this game).

@unknownbrackets
Copy link
Collaborator

Ugh, definitely could be NaNs. You can do this to check:

fpr.MapDirtyIn(fd, fs);
VCMP(fpr.R(fd), fpr.R(fs));
FixupBranch notNAN = B_CC(CC_VC);
Comp_Generic(op); // I think this will work, can't remember if any unlocks will be needed.
SetJumpTarget(notNAN)
// rest of your code

Anything that goes to interp is NaN. Could also be infinity I guess...

-[Unknown]

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

That didn't work either.

Something interesting...

This doesn't work (This is what should work):

    VCMP(S0, fpr.R(fs));
    SetCC(CC_GT); // CC_HI and CC_HS also fail! CC_GE as well

This does work (opposite?)

    VCMP(S0, fpr.R(fs));
    SetCC(CC_LT); // CC_LO and CC_LS also works! But not CC_LE

But this also doesn't work.

    VCMP(fpr.R(fs), S0);
    SetCC(CC_GE); // Also CC_GT fails

So interesting how S0 - R(fs) < 0 works but R(fs) - S0 >= 0 doesn't work...

@unknownbrackets
Copy link
Collaborator

It's confusing, but LT means "less than or one of them is NaN" and GT means "greater than and neither one is NaN". So this makes it sound like it really is a NaN issue.

Oh, my code above did not skip out of the regular method after Comp_Generic...

See here for unordered mappings:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Chdhcfbc.html

-[Unknown]

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

I got your code working by changing CC_VC to CC_VS.
Edit: Nevermind, doesn't make sense.

I updated those trials above with the ordered/unordered variants.

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

So I disabled the NaN detection in interpreter and everything worked just fine. Also, I tried CC_LO and it worked as well.
Looks like NaN is definitely not an issue.

So for some reason VCMP is working in only one direction. Time to check encoding?

@unknownbrackets
Copy link
Collaborator

My code should've been:

fpr.MapDirtyIn(fd, fs);
VCMP(fpr.R(fd), fpr.R(fs));
FixupBranch notNAN = B_CC(CC_VC);
Comp_Generic(op); // I think this will work, can't remember if any unlocks will be needed.
FixupBrand doneNAN = B();
SetJumpTarget(notNAN)
// rest of your code
SetJumpTarget(doneNAN);

Hmm, if VS made it work that is strange. Okay, so a trick I use here is to log everything. In ArmCompJit:

// At top of file.
int fpuFloorTemp;

...

    /*case 15: //FsI(fd) = (int)floorf(F(fs));      break; //floor.w.s
    {
        //fpr.MapDirtyIn(fd, fs);
        fpr.MapInIn(fd, fs);
        MOVI2F(S0, 0.4999999f, SCRATCHREG1);
        VSUB(S0,fpr.R(fs),S0);
        VCVT(fpr.R(fd), S0,        TO_INT | IS_SIGNED);
        MOVP2R(SCRATCHREG1, &fpuFloorTemp);
        VSTR(fpr.R(fd), SCRATCHREG1, 0);
        fpr.DiscardR(fd);
        DISABLE; // <-- send to interp after exec.
        break;
    }*/

    case 15: //FsI(fd) = (int)floorf(F(fs));      break; //floor.w.s
    {
        fpr.MapReg(fs);
        MOVI2F(S0, 0.4999999f, SCRATCHREG1);
        VSUB(S0, fpr.R(fs), S0);
        VCVT(S1, S0,        TO_INT | IS_SIGNED);
        MOVP2R(SCRATCHREG1, &fpuFloorTemp);
        VSTR(S1, SCRATCHREG1, 0);
        DISABLE; // <-- send to interp after exec.
        break;
    }

Then in interp:

// At top of file.
extern int fpuFloorTemp;

...
        case 15:
            if (my_isnanorinf(F(fs)))
            {
                int res = my_isinf(F(fs)) && F(fs) < 0.0f ? -2147483648LL : 2147483647LL;
                if (res != fpuFloorTemp) {
                    NOTICE_LOG(JIT, "armjit floor was wrong: %f -> %d instead of %d", F(fs), fpuFloorTemp, res);
                }
                FsI(fd) = res;
                break;
            }
...

            case 15: //FsI(fd) = (int)floorf(F(fs)); break; //floor.w.s
            {
                int res = (int)floorf(F(fs));
                if (res != fpuFloorTemp) {
                    NOTICE_LOG(JIT, "armjit floor was wrong: %f -> %d instead of %d", F(fs), fpuFloorTemp, res);
                }
                FsI(fd) = res;
                break;
            }

Untested. Edit: added nan/inf case.

-[Unknown]

@xsacha
Copy link
Collaborator

xsacha commented Jun 27, 2014

This is the code that works:

    fpr.MapDirtyIn(fd, fs);
    VCVT(fpr.R(fd), fpr.R(fs),        TO_INT | IS_SIGNED);
    VCVT(S0, fpr.R(fd),               TO_FLOAT | IS_SIGNED);
    VCMP(S0, fpr.R(fs));
    VMRS_APSR(); // Move FP flags from FPSCR to APSR (regular flags).
    SetCC(CC_GT);
    MOVI2F(fpr.R(fs), 1.0f, SCRATCHREG1);
    VSUB(S0, S0, fpr.R(fs));
    VCVT(fpr.R(fs), S0,               TO_INT | IS_SIGNED);
    SetCC(CC_AL);

Going now but I hope you can work out a better way :) Mostly for cvt.w.s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants