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

Build option to disable 64 Disk Drive support #649

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

None yet

4 participants