Sync dh fpu and normal fpu #2248

finalpatch · 2023-01-24T10:10:20Z

When the dynamic core uses normal core to run instructions, we need to

Copy the dh fpu state to normal fpu before entering the normal core
After returning from normal core, copy the normal fpu state to dh fpu

This addresses #2247

FeralChild64 · 2023-01-24T19:54:08Z

Indeed, the patch fixes the issue. Strangely, I can't reproduce the problem with 0.80.1, only with GIT master. And it is the guest software (Dos Navigator) which crashes, not DOSBox Staging itself (but I am using a Linux build):

finalpatch · 2023-01-24T21:02:56Z

@FeralChild64 this only happens if the dynamic core is using the dh fpu implementation. There are 3 different fpu code paths in dosbox, the normal core that interprets everything, the dynamic core without dh fpu that runs recompiled code but calls the normal fpu functions, and then the new dh fpu that does not share the same functions and data structures with normal core. I'm able to reproduce in windows stable release too.

Regarding whether it crashes dosbox or the guest app, I guess the overflow can happen in both normal core or dynamic core. If this happens in normal core it triggers the assertion, but if it happens in dynamic core, it'll probably trigger some sort of exception in the guest app.

src/cpu/core_dyn_x86.cpp

src/fpu/fpu.cpp

johnnovak · 2023-01-25T06:53:15Z

Do we notify the mainline DOSBox folks and @joncampbell123 out of courtesy @kcgen when a core emulation related important fix like this one lands in our repo?

kcgen · 2023-01-25T06:58:34Z

Yup, it's rare (that we have these), but I've previously informed QBix via PM. Will do the same when we eventually merge this.

Another route would be is if @finalpatch was planning to submit a .patch file to sourceforge (for SVN or 0.74.3).

kcgen · 2023-01-25T07:01:59Z

@finalpatch , all that's left is squashing all the intermediate commits down to one (I don't think we need to have this split across multiple.. it's a nice and small susinct change).

finalpatch · 2023-01-25T07:49:26Z

@finalpatch , all that's left is squashing all the intermediate commits down to one (I don't think we need to have this split across multiple.. it's a nice and small susinct change).

I saw some CI check failures, but they do not seem to be build or test failures.

finalpatch · 2023-01-25T07:51:30Z

Yup, it's rare (that we have these), but I've informed QBix via PM. Will do the same when we eventually merge this.

Another route would be is if @finalpatch was planning to submit a .patch file to sourceforge (for SVN or 0.74.3).

I can send them a patch file for sure. But it should be easy enough for them to just apply the changes themselves, it's not very large after all.

GranMinigun · 2023-01-25T13:29:41Z

Are there any performance implications for other software?

Grounded0 · 2023-01-25T13:52:46Z

For performance profiling my enhanced DOSBENCH can be found here:
https://anttipeltola.eu/misc/files/DOSBENCH.ZIP

kcgen · 2023-01-25T15:37:15Z

(text quote)

"On testing the latest commits in the above patch with core=dynamic, there is a significant
performance penalty, such as for win32 games as available to a Win95 guest OS. It may be
worth verifying in other environments. Otherwise, a better option is to test performance (and
the above fpu state issue) while running core=dynamic with a build based on the non-x86 fpu
(instead of the x86 fpu emulation)."

@finalpatch , have you found any DOS games that cause continuous cache lookup failures or have frequently-invalidated code blocks? Maybe self-modifying code would do this?

Unfortunately all of the above benchmarks never traverse this code path. I've also tested on x86-64 and ARM64 builds on native hardware using the dynamic and dynrec cores.

kcgen · 2023-01-25T17:09:59Z

I wasn't able to reproduce this with my existing copy of DOS Navigator.

I re-downloaded DN from https://dnosp.com/e_index.php today, just to start with a clean slate.

I grabbed the real-mode version (they start up faster and typically are smaller). Again.. couldn't reproduce it.

So I tried the DPMI version, and finally hit the issue.

finalpatch · 2023-01-26T04:12:08Z

@kcgen I changed the class and assignment call around CPU_Core_Normal_Run() to straight function calls.

There are a few other instances of CPU_Core_Normal_Run() in this file, I'm not entirely sure if they all need the same treatment. The fix is currently only applied to the one that was directly causing the problem that I can reproduce.

This reverts commit 85548f0.

This reverts commit 6638986.

finalpatch · 2023-01-26T08:38:04Z

Okay, answering my own question, we actually need to have this fpu sync around all the other CPU_Core_Normal_Run() calls.

Without these DOS debuggers won't be able to see FPU changes from dynamic core. After adding this, I'm now able to single step in Turbo Debugger and observe the FPU window updating in realtime on the dynamic core.

finalpatch · 2023-01-26T08:43:49Z

Turbo Debugger showing FPU on dynamic core (didn't work previously)

kcgen · 2023-01-26T09:27:17Z

Wonderful to see the turbo debugger working correctly now! This is the level of validation that shows this is right approach. 🚀

finalpatch · 2023-01-26T11:05:38Z

Unless you find new issues, I'm happy with the state of the PR. I'll leave it to you guys to merge it when you are ready.

finalpatch · 2023-01-26T11:14:13Z

There is definitely opportunity to clean up the FPU emulation code. I don't see any good reason to have 2 completely separate FPU states. Code would be much simpler and more efficient if we just have one.

kcgen

Thanks for this PR, @finalpatch, and working through the review. I learned a lot along the way - thanks for all the detailed explanations!

kcgen · 2023-01-26T12:02:25Z

Oh, one last change: would be able to squash the 14 commits down to one? I think one is enough to capture this change (and provide a bisectable point).

rderooy · 2023-01-26T12:34:46Z

FYI, I ran a benchmark on DOSBox-X with Win98. You can see the results here: joncampbell123/dosbox-x#3965 (comment)

finalpatch · 2023-01-26T13:54:08Z

Thanks for this PR, @finalpatch, and working through the review. I learned a lot along the way - thanks for all the detailed explanations!

I think you can configure github to squash merge pull requests:
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

I do not have merge permission so it's up to you how you want to merge it.

Grounded0 · 2023-01-26T14:17:01Z

@rderooy Thanks for running those benchmarks. We can absorb a hit on 16-bit clean and 16/32-bit mixed code but not for 32-bit clean code because that's where the most performance demand is. I approve based on evidence provided.

FeralChild64 · 2023-01-26T15:21:00Z

@kcgen IMHO this is a good candidate for backporting to 0.80.2 - we have no idea how many glitches or sporadic crashes this bug causes.

kcgen · 2023-01-26T17:19:35Z

I think you can configure github to squash merge pull requests. I do not have merge permission so it's up to you how you want to merge it.

We don't like using GitHub's auto-squash feature.

In part because it's a derivative: it's not what you (as the author) actually put forward for merging. Instead, it creates a huge list of the all the squashed commits.

In general, we want PRs to be in final shape, bisectable, and nicely named. Squashing isn't the norm, but it felt like the right granularity (for this PR) if anyone ever needs to bisect back to it.

There are merge conflicts, so GitHub is blocking on my side:

That's OK - I can do it manually. Here's a log (just for the record)

Checkout a new local branch on the main repo to hold your commits

$ git checkout -b fp/sync-fpu-1 main

Switched to a new branch 'fp/sync-fpu-1'

Import changes from your repo

git pull git@github.com:finalpatch/dosbox-staging.git main

remote: Enumerating objects: 75, done.
remote: Counting objects: 100% (75/75), done.
remote: Compressing objects: 100% (22/22), done.
remote: Total 75 (delta 55), reused 66 (delta 53), pack-reused 0
Unpacking objects: 100% (75/75), 18.75 KiB | 206.00 KiB/s, done.
From github.com:finalpatch/dosbox-staging
 * branch                main       -> FETCH_HEAD
Updating 0e791175e..b8b8157e8
Fast-forward
 include/fpu.h            | 13 +++++++++++++
 src/cpu/core_dyn_x86.cpp | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 src/fpu/fpu.cpp          | 12 ++++++++++++
 3 files changed, 76 insertions(+), 5 deletions(-)

Rebase your feature branch against main

git rebase -i main

pick 5d3e5b3f3 Sync dh fpu and normal fpu
pick bf0a01d6c refactor
pick ab3e77583 format
pick 6cac6456c fix unaligned access
pick c2b808652 fixes
pick 3a56fbf4a comments
pick 50695e7ea tighten the scope of auto_fpu_sync
pick 53037c2d7 fix formatting
pick 23b749aa0 refactor
pick 85548f04e revert incorrect index change
pick 6638986b6 Revert "revert incorrect index change"
pick f5f7d576f Revert "Revert "revert incorrect index change""
pick b8b8157e8 protected other instances of CPU_Core_Normal_Run()

Fix conflicts

Auto-merging src/cpu/core_dyn_x86.cpp
CONFLICT (content): Merge conflict in src/cpu/core_dyn_x86.cpp
error: could not apply 23b749aa0... refactor
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 23b749aa0... refactor

Squash intermediate commits

git rebase -i main

pick 69ef75db2 Sync dh fpu and normal fpu
squash e7f34e11c refactor
squash 779fabd8f format
squash aaab71ba2 fix unaligned access
squash c25f0f1a6 fixes
squash af2308099 comments
squash 326c8f3fd tighten the scope of auto_fpu_sync
squash 90b683c49 fix formatting
squash 945c281c6 refactor
squash e1986864a revert incorrect index change
squash fdc725bbc Revert "revert incorrect index change"
squash 0512a5564 Revert "Revert "revert incorrect index change""
squash df3b1b9fb protected other instances of CPU_Core_Normal_Run()

# Rebase 0e791175e..df3b1b9fb onto 0e791175e (13 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit

Reformat the commit message

Before

# This is a combination of 13 commits.
# This is the 1st commit message:

Sync dh fpu and normal fpu
# This is the commit message #2:

refactor

# This is the commit message #3:

format

# This is the commit message #4:

fix unaligned access

# This is the commit message #5:

fixes

# This is the commit message #6:

comments

# This is the commit message #7:

tighten the scope of auto_fpu_sync

# This is the commit message #8:

fix formatting

# This is the commit message #9:

refactor

# This is the commit message #10:

revert incorrect index change

# This is the commit message #11:

Revert "revert incorrect index change"

This reverts commit 85548f04e62f4d2b42cd415ad741f1faf4e1f842.

# This is the commit message #12:

Revert "Revert "revert incorrect index change""

This reverts commit 6638986b6bc1f4fd5810c3477d0efc4e39765855.

# This is the commit message #13:

protected other instances of CPU_Core_Normal_Run()

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#

After

Summarized from the PR discussion ...

Sync dh FPU and normal FPU (#2247)

When the dynamic core uses normal core to run instructions, we
need to:

1. Copy the dh FPU state to normal FPU before entering the
   normal core.

2. After returning from normal core, copy the normal FPU state
   to dh FPU.

Known affected programs (when using "core = dynamic"):

- DOS Navigator when repeating running bad commands on the
  command prompt.                       

- Turbo Debugger when showing the FPU state window and
  stepping forward line-by-line.             

This only happens if the dynamic core is using the dh FPU
implementation. There are three different FPU code paths in
DOSBox, the normal core that interprets everything, the
dynamic core without dh FPU that runs recompiled code but
calls the normal FPU functions, and then the new dh FPU that
does not share the same functions and data structures with
normal core.

This normal core run branch is triggered when the code block
was modified by the running program for more than 4 times. The
first 3 times the dyn core will just recompile the guest code.
Starting from the 4th time it will decide this part is
frequently changed so there is no point to refill the dynrec
cache and just call the normal core to run it.

Performance impact and testing:

- DOS games: Quake, Doom, Elite II (and more) plus all DOS
  benchmarks in PERFDOS2 did not follow this code path
  (presumably because they're not repeatedly modifying the same
  code block), and therefore there is no performance impact.

- Win98 tests by @rderooy using DOSBox-X show a ~20% and 5% 
  impact. https://github.com/joncampbell123/dosbox-x/pull/3965#issuec>

Future optimizations:

- The first syncing (from dh FPU to normal FPU) be made
  conditional based on the dynamic core's "state_used" flag
  (however, this flag is currently reset by the prior hardware
  FPU saver, so that needs fixing too).

- The second syncing (from normal FPU back to dh FPU) cannot 
  be made conditional yet because there is no flag indicating
  if the normal FPU was used, therefore this would need to be
  added.

Apply code formatting rules

./scripts/format-commit.sh

git version 2.37.2
Ubuntu clang-format version 16.0.0 (++20230115043638+df5fc4504b86-1~exp1~20230115163648.253)
Using paths relative to: /usr/src/dosbox-staging
clang-format -i -lines=118:122 -lines=128:135 -lines=145:147 "include/fpu.h"
clang-format -i -lines=251:269 -lines=285:285 -lines=300:304 -lines=313:333 -lines=390:396 -lines=419:419 -lines=421:421 "src/cpu/core_dyn_x86.cpp"
clang-format -i -lines=44:56 "src/fpu/fpu.cpp"

clang-format formatted some code for you.

Run 'git diff' to see what changed.
Run 'git commit -a --amend' to save the result.

Build and test

Also checked a UBSAN build -- all still running clean.

Sign off

git commit --amend --signoff

Perform a fast-forward merge to `main`

git merge --ff-only fp/sync-fpu-1

Updating 0e791175e..fd14ee5e4
Fast-forward
 include/fpu.h            | 19 ++++++++++++++++++-
 src/cpu/core_dyn_x86.cpp | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 src/fpu/fpu.cpp          | 14 ++++++++++++++

This produces a linear commit sequence on the main branch, without the merge commit appearing.

Push

git push

Ok - done.

kcgen · 2023-01-26T17:22:35Z

Merge performed manually outside of Github; closing.

finalpatch · 2023-01-26T22:32:28Z

@kcgen I just realise we need to move this

    if (dyn_dh_fpu.state_used) {
        gen_dh_fpu_save();
    }

into copy_dh_fpu_to_normal()
so that we can be sure it's copying the state is up to date.

Another thing is to put dh fpu related code inside a

#if defined(X86_DYNFPU_DH_ENABLED)

block so that code is still compilable when it's not defined (using the old dyn fpu)

See the other PR I made for X joncampbell123/dosbox-x#3969

kcgen · 2023-01-26T23:11:45Z

Great catches, @finalpatch.

finalpatch · 2023-01-27T01:16:53Z

Great catches, @finalpatch.

Would you be able to apply these changes?

kcgen · 2023-01-27T01:20:58Z

I'll try and then include you for review.

johnnovak · 2023-01-27T04:25:49Z

Nice work everyone! 💯 🚀

FeralChild64 · 2023-01-27T05:52:54Z

@finalpatch For the future:

It is best to keep your main exactly the same as Staging main, no additional commits
Features are best to be developed on branches, not on the main branch
Never merge the main branch to your feature branches, always do git rebase main while being on the feature branch

I did the mistake of developing a feature on the main branch too, in another project - the only reason it worked was because I was left as the only contributor.

johnnovak · 2023-01-27T06:58:05Z

You might want to familiarise yourself with the "Git rebase workflow" that we use @finalpatch

https://www.themoderncoder.com/a-better-git-workflow-with-rebase/

finalpatch mentioned this pull request Jan 24, 2023

Sync dh fpu state with normal fpu joncampbell123/dosbox-x#3965

Merged

FeralChild64 added the bug Something isn't working label Jan 24, 2023

FeralChild64 requested review from johnnovak and kcgen January 24, 2023 20:01

FeralChild64 assigned finalpatch Jan 24, 2023

FeralChild64 linked an issue Jan 24, 2023 that may be closed by this pull request

Need to sync dh_fpu and normal fpu when the dynamic core uses normal core to run some instructions #2247

Closed

3 tasks

kcgen reviewed Jan 25, 2023

View reviewed changes

src/cpu/core_dyn_x86.cpp Outdated Show resolved Hide resolved

kcgen reviewed Jan 25, 2023

View reviewed changes

src/cpu/core_dyn_x86.cpp Outdated Show resolved Hide resolved

kcgen reviewed Jan 25, 2023

View reviewed changes

src/cpu/core_dyn_x86.cpp Outdated Show resolved Hide resolved

kcgen reviewed Jan 25, 2023

View reviewed changes

src/fpu/fpu.cpp Outdated Show resolved Hide resolved

finalpatch added 7 commits January 25, 2023 18:47

Sync dh fpu and normal fpu

5d3e5b3

refactor

bf0a01d

format

ab3e775

fix unaligned access

6cac645

fixes

c2b8086

comments

3a56fbf

tighten the scope of auto_fpu_sync

50695e7

finalpatch force-pushed the main branch from 085199b to 50695e7 Compare January 25, 2023 07:47

fix formatting

53037c2

Merge remote-tracking branch 'origin/main'

3f3abb4

finalpatch added 3 commits January 26, 2023 18:30

revert incorrect index change

85548f0

Revert "revert incorrect index change"

6638986

This reverts commit 85548f0.

Revert "Revert "revert incorrect index change""

f5f7d57

This reverts commit 6638986.

protected other instances of CPU_Core_Normal_Run()

b8b8157

kcgen self-requested a review January 26, 2023 11:54

kcgen approved these changes Jan 26, 2023

View reviewed changes

kcgen closed this Jan 26, 2023

kcgen mentioned this pull request Feb 11, 2023

Update docs with supported versions of Windows, macOS, and Linux (#2258) #2276

Merged

Grounded0 added the CPU/FPU emulation Issues related to CPU or FPU emulation label Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync dh fpu and normal fpu #2248

Sync dh fpu and normal fpu #2248

finalpatch commented Jan 24, 2023

FeralChild64 commented Jan 24, 2023 •

edited

Loading

finalpatch commented Jan 24, 2023 •

edited

Loading

johnnovak commented Jan 25, 2023 •

edited

Loading

kcgen commented Jan 25, 2023 •

edited

Loading

kcgen commented Jan 25, 2023

finalpatch commented Jan 25, 2023

finalpatch commented Jan 25, 2023

GranMinigun commented Jan 25, 2023

Grounded0 commented Jan 25, 2023

kcgen commented Jan 25, 2023 •

edited

Loading

kcgen commented Jan 25, 2023 •

edited

Loading

finalpatch commented Jan 26, 2023

finalpatch commented Jan 26, 2023

finalpatch commented Jan 26, 2023

kcgen commented Jan 26, 2023

finalpatch commented Jan 26, 2023

finalpatch commented Jan 26, 2023

kcgen left a comment

kcgen commented Jan 26, 2023

rderooy commented Jan 26, 2023

finalpatch commented Jan 26, 2023 •

edited

Loading

Grounded0 commented Jan 26, 2023 •

edited

Loading

FeralChild64 commented Jan 26, 2023

kcgen commented Jan 26, 2023 •

edited

Loading

kcgen commented Jan 26, 2023

finalpatch commented Jan 26, 2023 •

edited

Loading

kcgen commented Jan 26, 2023

finalpatch commented Jan 27, 2023

kcgen commented Jan 27, 2023

johnnovak commented Jan 27, 2023

FeralChild64 commented Jan 27, 2023

johnnovak commented Jan 27, 2023

Sync dh fpu and normal fpu #2248

Sync dh fpu and normal fpu #2248

Conversation

finalpatch commented Jan 24, 2023

FeralChild64 commented Jan 24, 2023 • edited Loading

finalpatch commented Jan 24, 2023 • edited Loading

johnnovak commented Jan 25, 2023 • edited Loading

kcgen commented Jan 25, 2023 • edited Loading

kcgen commented Jan 25, 2023

finalpatch commented Jan 25, 2023

finalpatch commented Jan 25, 2023

GranMinigun commented Jan 25, 2023

Grounded0 commented Jan 25, 2023

kcgen commented Jan 25, 2023 • edited Loading

kcgen commented Jan 25, 2023 • edited Loading

finalpatch commented Jan 26, 2023

finalpatch commented Jan 26, 2023

finalpatch commented Jan 26, 2023

kcgen commented Jan 26, 2023

finalpatch commented Jan 26, 2023

finalpatch commented Jan 26, 2023

kcgen left a comment

Choose a reason for hiding this comment

kcgen commented Jan 26, 2023

rderooy commented Jan 26, 2023

finalpatch commented Jan 26, 2023 • edited Loading

Grounded0 commented Jan 26, 2023 • edited Loading

FeralChild64 commented Jan 26, 2023

kcgen commented Jan 26, 2023 • edited Loading

Checkout a new local branch on the main repo to hold your commits

Import changes from your repo

Rebase your feature branch against main

Fix conflicts

Squash intermediate commits

Reformat the commit message

Before

After

Apply code formatting rules

Build and test

Sign off

Perform a fast-forward merge to main

Push

kcgen commented Jan 26, 2023

finalpatch commented Jan 26, 2023 • edited Loading

kcgen commented Jan 26, 2023

finalpatch commented Jan 27, 2023

kcgen commented Jan 27, 2023

johnnovak commented Jan 27, 2023

FeralChild64 commented Jan 27, 2023

johnnovak commented Jan 27, 2023

FeralChild64 commented Jan 24, 2023 •

edited

Loading

finalpatch commented Jan 24, 2023 •

edited

Loading

johnnovak commented Jan 25, 2023 •

edited

Loading

kcgen commented Jan 25, 2023 •

edited

Loading

kcgen commented Jan 25, 2023 •

edited

Loading

kcgen commented Jan 25, 2023 •

edited

Loading

finalpatch commented Jan 26, 2023 •

edited

Loading

Grounded0 commented Jan 26, 2023 •

edited

Loading

kcgen commented Jan 26, 2023 •

edited

Loading

Perform a fast-forward merge to `main`

finalpatch commented Jan 26, 2023 •

edited

Loading