Skip to content

Conversation

@hissingshark
Copy link

There is a longstanding issue since the addition of the 64 Disk Drive support. This is causing some ARM devices to segfault at emulator start.

This commit adds a build option 64DISKDRIVE.
It defaults to 64DISKDRIVE=1 as I expect most devices are unaffected.
Those platforms vulnerable to it can use 64DISKDRIVE=0.

Many Thanks

@hissingshark
Copy link
Author

Ah, sorry forgot about the Visual C++ files:

  • mupen64plus-core.vcxproj
  • mupen64plus-core.vcxproj.filters

I think those explain the AppVeyor build fail. Not familiar with configuring those and no way to test locally (no Windows box here) so will need to read up.

@hissingshark hissingshark force-pushed the buildoption branch 4 times, most recently from 0ad88dc to 6d116fd Compare May 30, 2019 08:44
@hissingshark hissingshark changed the title Build option to toggle 64 Disk Drive support Build option to disable 64 Disk Drive support May 30, 2019
@hissingshark hissingshark reopened this May 30, 2019
@hissingshark
Copy link
Author

It seemed like there was a problem with the checks for desktop builds because the Makefile wasn't setting the pre-processor macro, but now I'm not so sure.
Regardless I've started again and made this a "disable feature" build option. Now checking successfully.

NO_64DD=1 == build without 64 Disk Drive support

I hope this feature is acceptable.

@richard42
Copy link
Member

I'm okay with this capability in general (being able to disable 64DD support at build time) but I have a problem with this implementation. There are 50 new #ifdef macro blocks in this merge request (that's a lot), and the function interfaces (internal API) are different depending upon whether or not this macro is defined.

There must be a simpler way to achieve this with fewer #ifdefs, like setting the dd_controller pointer to NULL if it's not supported, and keeping the existing internal API.

@loganmc10
Copy link
Member

I think it would be a lot better to figure out why the new dynarec crashes with 64DD support, there shouldn't really be a reason to remove DD support like this

@fzurita
Copy link
Member

fzurita commented Dec 30, 2020

Also, Android does not appear to have this problem. I agree with @loganmc10, if @hissingshark could provide a backtrace to the core dump, I'm sure the issue could be figured out.

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.

4 participants