mupen64plus-core icon indicating copy to clipboard operation
mupen64plus-core copied to clipboard

Misc crash fixes

Open Rosalie241 opened this issue 9 months ago • 5 comments

Fixes #1050 Fixes #1051 Fixes #1052

cc @m4xw for the new_dynarec changes cc @loganmc10 for the register bounds check changes

Rosalie241 avatar May 08 '24 18:05 Rosalie241

For the out-of-bounds register access, what happens on an actual N64?

I assume that writes to non-existent registers are just ignored, but the way this is written, a read leaves the value as-is. I'm not sure this is the proper behaviour, maybe it returns a 0? Or maybe the N64 would crash? I'm not sure

loganmc10 avatar May 08 '24 18:05 loganmc10

Good question, n64brew doesn't seem to mention such behavior so I've pinged Rasky on Discord already, though before this patch, out-of-bounds reads didn't read a correct value anyways, and out-of-bounds writes could cause crashes, so this patch just makes that safer, but I agree we should make it accurate if someone tests it on an actual N64 and reports back.

Rosalie241 avatar May 08 '24 18:05 Rosalie241

It seems that some registers do wrap around, but n64-systemtest has no tests for this currently, there's a feature request open for that though, so I feel more comfortable trying to implement that when there are tests verifying that behavior to ensure I don't break anything.

I still feel like my commit is safe because it just prevents out-of-bounds access, which could cause crashes in some cases, like demonstrated in the issues I've linked in the description of this pull request, it didn't work accurate to hardware before or after this change, so accuracy wise nothing has changed.

Rosalie241 avatar May 11 '24 21:05 Rosalie241

But it's just trading one inaccurate behavior for another. The only thing that crashes are some test ROMs. If the registers do really wrap around then it would be better to implement that behavior.

loganmc10 avatar May 11 '24 23:05 loganmc10

It would also be a simpler change to implement the register wrap-around in 2 places in the inline register calculation functions in the headers, rather than in every place that uses those functions.

richard42 avatar May 21 '24 04:05 richard42