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

Extra result handling from funcStat CB in savedata_op #5399

Merged
merged 1 commit into from
Dec 8, 2018

Conversation

RipleyTom
Copy link
Contributor

@RipleyTom RipleyTom commented Dec 4, 2018

The funcStat callback is called even if the directory doesn't exist.
Its returning statSet->reCreateMode doesn't seem to change the resulting behaviour as per my testing on real HW.

REd library:
CELL_SAVEDATA_CBRESULT_OK_LAST_NOCONFIRM returns CELL_SAVEDATA_ERROR_PARAM
CELL_SAVEDATA_CBRESULT_OK_LAST returns CELL_OK

Both instantly terminate after calling funcStat.

@RipleyTom
Copy link
Contributor Author

Fixes some games(Ghost of Sparta, maybe others) having no sound on first load because savedata_op would not return an error in this particular case.

@@ -610,6 +610,12 @@ static NEVER_INLINE s32 savedata_op(ppu_thread& ppu, u32 operation, u32 version,
// //return CELL_OK;
//}

if (operation == SAVEDATA_OP_AUTO_LOAD && !fs::is_dir(dir_path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SAVE vs LOAD wasn't supposed to have any effect on function behaviour except visual.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right cellSaveDataAutoLoad2 has the same exact behavior as cellSaveDataAutoSave2.
I'll investigate a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further testing it seems it's the CB returning CELL_SAVEDATA_CBRESULT_OK_LAST_NOCONFIRM that result in an error.

Strangely enough the CB returning CELL_SAVEDATA_CBRESULT_OK_LAST terminates the function with a CELL_OK.

@RipleyTom RipleyTom force-pushed the loaderror branch 2 times, most recently from f5dbb1a to eaee477 Compare December 4, 2018 19:33
@RipleyTom RipleyTom changed the title SAVEDATA_OP_AUTO_LOAD fixup Extra result handling from funcStat CB in savedata_op Dec 4, 2018
@RipleyTom
Copy link
Contributor Author

Just in case I did extra checking:
seg000:0000D7A4 bctrl # funcStat!
seg000:0000D7A8 ld r2, 0xD70+var_D48(r1)
seg000:0000D7AC lwz r7, 0xD70+var_CA0(r1)
seg000:0000D7B0 addi r0, r7, 5 # switch 7 cases
seg000:0000D7B4 lwz r10, 0xD70+var_C90(r1)
seg000:0000D7B8 cmplwi cr7, r0, 6
seg000:0000D7BC stw r10, 4(r18)
seg000:0000D7C0 bgt cr7, def_E290 # jumptable 0000D7E0 default case

CELL_SAVEDATA_CBRESULT_OK_LAST_NOCONFIRM = 2
It adds 5, so 7, jumps to default which returns a param error.

1(aka 6) leads to termination with CELL_OK and otherwise follow the same path.

There is a function at seg000:0000CBF0 bl do_stuff_3 that does some extra stuff before quitting but I thing it just cleans the cxml session.

@Nekotekina
Copy link
Member

Then this is a fallback for all values >= 2 or less than -5

@RipleyTom RipleyTom force-pushed the loaderror branch 2 times, most recently from 8448148 to 940a9dd Compare December 4, 2018 23:04
@RipleyTom
Copy link
Contributor Author

You're right, fixed.

if (result->result < CELL_SAVEDATA_CBRESULT_OK_NEXT)
{
return CELL_SAVEDATA_ERROR_CBRESULT;
}

//Skip and return without error
if (result->result == CELL_SAVEDATA_CBRESULT_OK_LAST)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this part (see #4397)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's not correct, I'll take a look at the game but from what I see in disassembly it terminates without calling any other callback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does terminate later, it's already handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reversed Battle Princess of Arcadias, the launch race condition has nothing to do with save functions, no reason to keep the hack in.

@HerrHulaHoop
Copy link

This PR fixes #4985
rpcs3_2018-12-05_15-00-00

On latest master, I still see the MEM Access Violation as mentioned in the issue. With this PR, I get past the error and comfortably ingame!

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 this pull request may close these issues.

3 participants