This reverts commit 72cf2bdb87.
SYSCONF settings are getting cleared when they shouldn't be. Let's
revert the change until I get proper time to figure out why it's broken.
Some pieces of code are calling IsRunning because there's some
particular action that only makes sense when emulation is running, for
instance showing the state of the emulated CPU. IsRunning is appropriate
to use for this. Then there are pieces of code that are calling
IsRunning because there's some particular thing they must avoid doing
e.g. when the CPU thread is running or IOS is running. IsRunning isn't
quite appropriate for this. Such code should also be checking for the
states Starting and Stopping. Keep in mind that:
* When the state is Starting, the state can asynchronously change to
Running at any time.
* When we try to stop the core, the state gets set to Stopping before we
take any action to actually stop things.
This commit adds a new method Core::IsUninitialized, and changes all
callers of IsRunning and GetState that look to me like they should be
changed.
Core::GetState reads from four different pieces of state: s_is_stopping,
s_hardware_initialized, s_is_booting, and CPUManager::IsStepping.
I'm keeping that last one as is for now because there's code in Dolphin
that sets it directly, but we can unify the other three to make things
easier to reason about.
This commit also gets rid of s_is_started. This was previously used in
Core::IsRunningAndStarted to ensure true wouldn't be returned until the
CPU thread was started, but it wasn't used in Core::GetState, so
Core::GetState would happily return State::Running after we had
initialized the hardware but before we had initialized the CPU thread.
As far as I know, there are no callers that have any real need to know
whether the boot process is currently initializing the hardware or the
CPU thread. Perhaps once upon a time there was a desire to make the
apploader debuggable, but a long time has passed without anyone stepping
up to implement it, and the way CBoot::RunApploader is implemented makes
it rather difficult. So this commit makes all the functions in Core.cpp
consider the core to still be starting until the CPU thread is started.
We currently have 32 different binaries containing unit tests. At least
when I build for Android, each one takes up over 200 MiB, and linking
them all increases my incremental build times by over a minute. I'd
like to change this for the sake of my productivity and disk space.
For reference, MSBuild is already putting all tests in a single binary.
This lets us reduce the number of USE_RETRO_ACHIEVEMENTS ifdefs in the
code base, reducing visual clutter. In particular, needing an ifdef for
each call to IsHardcodeModeActive was annoying to me. This also reduces
the risk that someone writes code that accidentally fails to compile
with USE_RETRO_ACHIEVEMENTS disabled.
We could cut down on ifdefs even further by making HardcodeWarningWidget
always exist, but that would result in non-trivial code ending up in the
binary even with USE_RETRO_ACHIEVEMENTS disabled, so I'm leaving it out
of this PR. It's not a lot of code though, so I might end up revisiting
it at some point.
Right now, we assign a versionCode to each Android build of Dolphin by
counting the total number of git commits made. This has worked fine so
far, but it won't work as-is for the new release process.
Let's say we're currently on commit 20000. If we want to create a
release under the new release process, we would create a release branch,
add a new commit on it that updates the release name in CMake files and
so on, and create a tag for that commit. The Android build of this
release commit would get the version code 20001. However, the master
branch is also going to get a commit with the version code 20001 sooner
or later, and this commit would be an entirely different commit than
commit 20001 on the release branch. This isn't much of a problem for
people downloading Dolphin from dolphin-emu.org, but it's a big problem
for Google Play, as Google Play doesn't allow us to upload two builds
with the same version code.
This commit makes us calculate the Android version code in a new way:
The number of commits times two, and if the current build isn't a
release build, plus 1. (We check whether the current build is a release
build by checking whether there's a tag for the current commit.)
With this new version code scheme, the release commit described in my
example would get the version code 40002, and the master commit would
get the version code 40003. This lets us upload both corresponding
builds to Google Play, and also lets the user switch from the release
build to the development build if they would like to. (Under normal
circumstances, Android forbids installing a build with an older version
code than the currently installed build. Therefore, whether the 1 is
added for release builds or for development builds is a decision with
consequences.)
Using shifts and bit tests makes the code unnecessarily annoying to
reason about. I'm replacing it with subtracting from 3 to translate the
bit order from the PowerPC format to the usual format.
BI contains both the field and the flag (5 bits total), so we need to
shift away the 2 flag bits to get the 3 field bits. (Same as the
CRBA/CRBB handling in the code just below the BI code.)
This is a JitArm64 version of 219610d8a0.
Due to limitations on how far you can jump with a single AArch64 branch
instruction, going above the former limit of 128 MiB of code (counting
nearcode and farcode combined) requires a bit of restructuring. With the
restructuring in place, the limit now is 256 MiB. See the new large
comment in Jit.h for a description of the new memory layout.
To ensure memory safety, callers of GetPointer have to perform a bounds
check. But how is this bounds check supposed to be performed?
GetPointerForRange contained one implementation of a bounds check, but
it was cumbersome, and it also isn't obvious why it's correct.
To make doing the right thing easier, this commit changes GetPointer to
return a span that tells the caller how many bytes it's allowed to
access.
This fourth part of my series of patches to get rid of unsafe uses of
GetPointer takes care of the "easy" cases in VideoCommon. Three uses of
GetPointer now remain in Dolphin: VertexLoaderManager, TextureInfo, and
the software renderer's TextureSampler.
When writing the software FMA code, I didn't realize that we can't
overwrite d if d is the same register as one of the inputs and
HandleNaNs is going to be called. This fixes that.
It was possible for sAlertMessageLock.notify() to be called before
sAlertMessageLock.wait(), causing Dolphin to deadlock. In particular,
this was guaranteed to happen if displayAlertMsg was called from the UI
thread while the emulation activity is being destroyed, because
runOnUiThread runs the passed-in anonymous function immediately when
called from the UI thread.
By replacing Object.wait/Object.notify with Semaphore.acquire/
Semaphore.release, it no longer matters what order the methods are
called in.
Because SettingViewHolder is used in RecyclerViews, we have to
explicitly unset STRIKE_THRU_TEXT_FLAG when we don't want it, otherwise
it might be left over from when the SettingViewHolder was representing
a different setting.
IOS::HLE::IOCtlVRequest::Dump sometimes tries to call GetPointerForRange
with an address of 0 and a size of 0. Address 0 is valid, but we were
mistakenly also trying to check that address 3FFFFFFF is valid, which it
isn't.
Fixes https://bugs.dolphin-emu.org/issues/13514.
Typically when someone uses GetPointer, it's because they want to read
from a range of memory. GetPointer is unsafe to use for this. While it
does check that the passed-in address is valid, it doesn't know the size
of the range that will be accessed, so it can't check that the end
address is valid. The safer alternative GetPointerForRange should be
used instead.
Note that there is still the problem of many callers not checking for
nullptr.
This is part 2 of a series of changes removing the use of GetPointer
throughout the code base. After this, VideoCommon is the one major part
of Dolphin that remains.
Typically when someone uses GetPointer, it's because they want to read
from a range of memory. GetPointer is unsafe to use for this. While it
does check that the passed-in address is valid, it doesn't know the size
of the range that will be accessed, so it can't check that the end
address is valid. The safer alternative GetPointerForRange should be
used instead.
Note that there is still the problem of many callers not checking for
nullptr.
This is the first part of a series of changes that will remove the usage
of GetPointer in different parts of the code base. This commit gets rid
of every GetPointer call from our IOS code except for a particularly
tricky one in BluetoothEmuDevice.
RCOpArg::ExtractWithByteOffset is only used in one place: a special case
of rlwinmx. ExtractWithByteOffset first stores the value of the
specified register into m_ppc_state (unless it's already there), and
then returns an offset into m_ppc_state. Our use of this function has
two undesirable properties (except in the trivial case `offset == 0`):
1. ExtractWithByteOffset calls StoreFromRegister without going through
any of the usual functions. This violated an assumption I made when
working on my constant propagation PR and led to a hard-to-find bug.
2. If the specified register is in a host register and is dirty,
ExtractWithByteOffset will store its value to m_ppc_state even when
it's unnecessary. In particular, this always happens when rlwinmx
uses the same register as input and output, since rlwinmx always
allocates a host register for the output and marks it as dirty.
Since ExtractWithByteOffset is only used in one place, I figure we might
as well inline it there. This commit does that, and also alters
rlwinmx's logic so the special case code is only triggered when the
input is already in m_ppc_state.
Input in `m_ppc_state`, before (11 bytes):
mov esi, dword ptr [rbp-104]
mov dword ptr [rbp-104], esi
movzx esi, byte ptr [rbp-101]
Input in `m_ppc_state`, after (5 bytes):
movzx esi, byte ptr [rbp-101]
Input in host register, before (8 bytes):
mov dword ptr [rbp-104], esi
movzx esi, byte ptr [rbp-101]
Input in host register, after (3 bytes):
shr edi, 0x18
Not sure if the behavior I'm implementing here is what real hardware
does, but since this is a buffer overflow, I'd like to get it fixed
quickly. Hardware verification can happen later.
https://bugs.dolphin-emu.org/issues/13506
Having to look up macros that are defined elsewhere makes the code
harder to reason about. The macros don't remove enough repetition to
justify their existence in my opinion.
With this, I intend to make it clearer that Auto, Force 4:3, Force 16:9
and Custom are really the same thing, just with the aspect ratio of the
simulated TV being selected in a different way. I also extended the
introduction in a way I feel will clarify things but which you are
welcome to bikeshed :)
I was thinking of this during the review of 41b19e262f, but wanted to
put it in a separate PR as to avoid blocking it on bikeshedding.
I'm a bit unsure what to do about the word "analog" in "analog TV". I
felt that repeating it for each of these options would be too
repetitive. I suppose there's a reason why we used the word originally,
but digital TVs do give you basically the same aspect ratio for GC/Wii
games as analog TVs. (Of course, whether it's 4:3-like or 16:9-like
depends on what aspect ratio you set in the TV's settings, but that's
the case for widescreen CRTs too.)
Google Play's policies require us to tell the user the size of any large
download.
The size seems to vary by just a megabyte or two across regions in my
testing, so I'm listing a rough size for all the regions.
I'm also taking the opportunity to shorten the message to make it easier
to read.
Because the wording of the Load Wii System Menu string can change
depending on the contents of the NAND, we should update that menu item in
a method that's guaranteed to get called every time the user opens the
menu rather than one that's only guaranteed to be called once.
Unlike ADD (immediate), MOV (register) treats SP as ZR. Therefore the
ADDI2R optimization that was added in 67791d227c can't optimize ADD to
MOV when exactly one of the registers is SP.
There currently isn't any code in Dolphin that calls ADDI2R with
parameters that would trigger this case.
Because the last commit made us use separate folders for GCPad and
GCKey profiles, we should also use separate game INI keys for them.
Otherwise setting e.g. PadProfile1 in a game INI will make both GCPad
and GCKey try to load it, typically with one of them succeeding and the
other one showing a panic alert due to the profile not existing in its
folder.
Better do this breaking change for GCKeys in the same PR as the other
breaking change rather than later.
After reading the previous commit, you might think "hold on, what's the
difference between GetProfileName and GetProfileDirectoryName"? These
two are being used for the exact same thing - figuring out where
profiles are stored - yet they return different values for certain
controllers like GC keyboards! As far as I can tell, the existing code
has been broken for GC keyboards since they were introduced a decade
ago. The GUI (and more recently, also InputCycler) would write and read
profiles in one location, and our code for loading profiles specified in
a game INI file would read profiles in another location.
This commit gets rid of the set of values used by the game INI code in
favor of the other set. This does breaking existing setups where a
GCKey profile has been configured in a game INI, but I think the number
of working such setups is vanishingly small. The alternative would make
existing GCKey profiles go missing from the profile dropdown in the GUI,
which I think would be more disruptive. The alternative would also force
new GCKey profiles into the same directory as GCPad profiles.
This commit also fixes a regression from d6c0f8e749. The Android GUI was
using GetProfileName to figure out what key to use in the game INI,
which made it use incorrect game INI entries for GameCube controller
profiles but not Wii Remote profiles. Now the Android GUI uses
GetProfileKey for this, fixing the problem.
By having getters for this information, other code that needs access to
the same information can call the getters instead of duplicating the
information.
This reverts the parts of commit c8c9928eb1 that made translatability
worse rather than better. Changing "Error in column %2" to "%1 in column
%2" not only means that the translators have to check the i18n comments
to know what word hides behind %1, but there's also the problem that
the translator might need to translate "Error" in this context
differently from the standalone string "Error". Having to copy-paste
some HTML tags may be annoying for translators, but it's a far less
serious problem.
This is another one of those games that has an optional screenshot
feature where the images end up all black if you have Store EFB Copies
to Texture Only turned on. Like for other such games, let's not force
Store EFB Copies to Texture Only off, since it's a large performance
impact for a feature most players won't use.
There's one wrinkle here. As part of teaching the player how to take
screenshots, the game forces the player to take a screenshot before it
lets them progress in the story. However, the game doesn't care what's
in the screenshot, so you can progress just fine even if Store EFB
Copies to Texture Only is turned on. I personally tested it.
So somehow I forgot that AArch64 uses three-operand encoding...
Fixes a regression from 6303416201 which manifested in various ways,
such as incorrect rendering of the Wind Waker title screen.
The codegen for the functions themselves, not for the emitted code.
This seems to save 32 bytes per function. We also get rid of the oddity
we had before where ANDI2R would do masking for 32-bit operations but
the other functions wouldn't.
JosJuice