mupen64plus / mupen64plus-core

Core module of the Mupen64Plus project
1.25k stars 254 forks source link

Misc crash fixes #1080

Open Rosalie241 opened 1 month ago

Rosalie241 commented 1 month ago

Fixes #1050 Fixes #1051 Fixes #1052

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

loganmc10 commented 1 month ago

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

Rosalie241 commented 1 month ago

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 commented 1 month ago

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.

loganmc10 commented 1 month ago

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.

richard42 commented 1 month ago

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.