64-bit dynamic_x86 (patch)

Developer's Forum, for discussion of bugs, code, and other developmental aspects of DOSBox.

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-7-03 @ 15:30

Doesn't make sense to me - a 32-bit build both with and without this patch should have X86_DYNFPU_DH_ENABLED defined (and the official release builds definitely do).
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby hail-to-the-ryzen » 2019-7-03 @ 16:37

Thank you again for the kind help.
hail-to-the-ryzen
Member
 
Posts: 342
Joined: 2017-3-09 @ 01:34

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-7-16 @ 00:12

So I assume there's no more "illegal option" errors with the latest patch?
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby robertmo » 2019-9-28 @ 07:57

time for core haxm :)
User avatar
robertmo
l33t
 
Posts: 4792
Joined: 2003-6-18 @ 10:35

Re: 64-bit dynamic_x86 (patch)

Postby Qbix » 2019-9-30 @ 18:44

Added in 4260.
Water flows down the stream
How to ask questions the smart way!
User avatar
Qbix
DOSBox Author
 
Posts: 10947
Joined: 2002-11-27 @ 14:50
Location: Fryslan

Re: 64-bit dynamic_x86 (patch)

Postby dreamer_ » 2019-9-30 @ 21:39

Hi, I just imported r4260 and r4261 to dosbox-staging - these new changes introduced a bunch of new warnings (+31 - when compiling with on GCC9 with -Wall) and a several (+7) new issues detected by static code analysis.

You can check full CI logs by here:
before merging-in new changes from SVN: https://github.com/dreamer/dosbox-stagi ... /239673808 (you can click on "Summarize warnings" to see the warning count).
after: https://github.com/dreamer/dosbox-stagi ... /242223652

Link to full static analysis report after the merge: https://github.com/dreamer/dosbox-stagi ... acts/69504 (you can find full report for every build in Artifacts in job called "Clang static analyzer").
Attachments
new-warnings.txt
(9.5 KiB) Downloaded 7 times
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-10-01 @ 05:46

The only static analysis issue that looked remotely valid to me was "Called C++ object pointer is null" for line 444 in risc_x64.h, but the analyzer has performed an invalid simulation (assumes dnew->genreg != dsynch->genreg but also assumes both values are NULL, making the first assumption invalid).
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby dreamer_ » 2019-10-01 @ 07:10

With static code analysis there's always a risk of false-positives, but I think this is not such case.

Information for line 434 refers to the value of `dnew->genreg` after Clear (this pointer is set to 0 inside Clear if - I assume that's the case here - there's a cyclic dependency between dynrec and genrec (line 244)). Therefore whenever this code will start with (`dsynch->genreg == nullptr` and `dnew->genreg != nullptr`), then there's real possibility of dereferencing null pointer in line 444.

I agree, that presentation of report is misleading for line 434, but the analysis seems correct to me. I did not look in detail into other new static analysis issues, but in my experience machine is quite often right.

Pasted analysis, so people won't need to click around:
Code: Select all
431   static void gen_synchreg(DynReg * dnew,DynReg * dsynch) {
432      /* First make sure the registers match */
433      if (dnew->genreg!=dsynch->genreg) {
   Assuming the condition is true   
434         if (dnew->genreg) dnew->genreg->Clear();
   Assuming pointer value is null   
435         if (dsynch->genreg) {
   Assuming the condition is false   
   Taking false branch   
436            dsynch->genreg->Load(dnew);
437         }
438      }
439      /* Always use the loadonce flag from either state */
440      dnew->flags|=(dsynch->flags & dnew->flags&DYNFLG_ACTIVE);
441      if ((dnew->flags ^ dsynch->flags) & DYNFLG_CHANGED) {
   Taking true branch   
442         /* Ensure the changed value gets saved */   
443         if (dnew->flags & DYNFLG_CHANGED) {
   Assuming the condition is true   
   Taking true branch   

444            dnew->genreg->Save();
   Called C++ object pointer is null
445         } else dnew->flags|=DYNFLG_CHANGED;
446      }
447   }
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-10-01 @ 08:13

dreamer_ wrote:Information for line 434 refers to the value of `dnew->genreg` after Clear (this pointer is set to 0 inside Clear if - I assume that's the case here - there's a cyclic dependency between dynrec and genrec (line 244)). Therefore whenever this code will start with (`dsynch->genreg == nullptr` and `dnew->genreg != nullptr`), then there's real possibility of dereferencing null pointer in line 444.

Comments in yellow apply to previous code lines, not the lines after them. You can tell this by the underlining of the relevant conditions/pointers.
#5 corresponds to line 433 (assuming dnew->genreg != dsynch->genreg).
#7 corresponds to line 434 (assuming dnew->genreg is NULL/false and not executing dnew->genreg->Clear()
#9 corresponds to line 435 (assuming dsync->genreg is NULL/false and not executing dsynch->genreg->Load(dnew))
If dnew->genreg->Clear() was assumed to be part of the execution it would be included in the analysis, it's not a virtual method. And if that were the case it would be further incorrect to assume dnew->genreg is now NULL, but not realize DYNFLAG_CHANGED has also been cleared.

Whenever genreg is set to NULL, DYNFLG_CHANGED is cleared so it's definitely a static analysis fail.
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby dreamer_ » 2019-10-01 @ 08:35

I don't understand where you get an assumption that DYNFLAG_CHANGED is cleared - I don't see this in code? (but I'm not the author here - I might be missing something) There's nothing in GenRec::Clear() function to indicate that flag is cleared when pointer is being set to 0. Also, if proving that this code is ok requires the reader to jump around the file, then maybe introducing a null check or at least an assert just in case some future change will blow up in here is a good idea?

Your patch is very much appreciated, but I care about improving the quality of DOSBox code and not only features - it would be nice to take the number of warnings down instead of introducing new ones. I also understand it's hard to follow all warnings and static analysis when there's no CI set up, but - well, now there is one.
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-10-01 @ 08:50

GenReg::Clear() checks if DYNFLG_CHANGED is set. If so it calls Save(), which clears it.

This isn't new code, it's copied verbatim from risc_x86.h. Maybe you should improve your CI workflow to build the most popular DOSBox configuration (32-bit + dyn_x86).
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby dreamer_ » 2019-10-01 @ 10:18

jmarsh wrote:GenReg::Clear() checks if DYNFLG_CHANGED is set. If so it calls Save(), which clears it.


Ok, I see it now, but it is really hard to follow - and if someone touches this part of code without knowledge of this interaction, or if the behaviour of code changes slightly in the future, it might introduce a crash. I would recommend placing *at least* an assert:

Code: Select all
risc_x64.h:443 +   assert((dnew->flags & DYNFLG_CHANGED) && (dnew->genreg != nullptr));
risc_x64.h:444     if (dnew->flags & DYNFLG_CHANGED) {
risc_x64.h:445        dnew->genreg->Save();
risc_x64.h:446     } else dnew->flags|=DYNFLG_CHANGED;


(I would still prefer a nullcheck though)

Or maybe an assert in GenReg would be more appropriate:

Code: Select all
risc_x64.h:263     void Clear(void) {
risc_x64.h:264        if (!dynreg) return;
risc_x64.h:265        if (dynreg->flags&DYNFLG_CHANGED) {
risc_x64.h:266           Save();
risc_x64.h:267        }
risc_x64.h:268        dynreg->genreg=0;dynreg=0;
risc_x64.h:269 +      assert(!(dynreg->flags & DYNFLG_CHANGED));
risc_x64.h:270     }


jmarsh wrote:This isn't new code, it's copied verbatim from risc_x86.h.


Uhmm, then maybe the original function should not be static, but implementation should be common for both files instead of introducing code duplication?

jmarsh wrote:Maybe you should improve your CI workflow to build the most popular DOSBox configuration (32-bit + dyn_x86).


Outside of Windows, 64-bit is the most common configuration, but I agree - this is an excellent idea, I will implement 32-bit build and static analysis on CI, after MSVC builds will be finished. Thanks, for the suggestion :)
Code: Select all
| ← Ceci n'est pas une pipe
User avatar
dreamer_
Newbie
 
Posts: 43
Joined: 2019-5-17 @ 20:19

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-10-02 @ 14:00

For anyone testing 64-bit builds on windows, SDL can have a minor issue where it causes a crash in a message handler and the OS "helpfully" tries to permanently apply a compatibility fix to hide it.
This was fixed in a later SDL-1.2 version and can be backported as a workaround: https://hg.libsdl.org/SDL/diff/daf9e603 ... x5events.c
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby Kerr Avon » 2019-10-05 @ 12:33

On behalf of the non-techy members of Vogons, can I ask which version of DOSBox runs the fastest now, the original 32 bit, or this 64 bit build (is the one in the first post of this topic still the latest one?), and is that the same DOSBox as v0.74-3, or does it have any patches and newer functions added?
Kerr Avon
Oldbie
 
Posts: 551
Joined: 2007-6-29 @ 14:33

Re: 64-bit dynamic_x86 (patch)

Postby Dominus » 2019-10-05 @ 12:50

the 32bit one is still the fastest. So if you run DOSBox 0.74-3 you are good. But some operating systems are doing away with 32bit support and before this patch 64bit DOSBox was really way slower. This patch is awesome for those OS :)
The patch is now in SVN so up to date SVN builds will have it and are way newer than the binary in the first post.
Hope this helps.
User avatar
Dominus
DOSBox Moderator
 
Posts: 8006
Joined: 2002-10-03 @ 09:54
Location: Ludwigsburg

Re: 64-bit dynamic_x86 (patch)

Postby Firtasik » 2019-10-05 @ 12:59

Dominus wrote:the 32bit one is still the fastest.
Nope.
11 1 111 11 1 1 1 1 1 11 1 1 111 1 111 1 1 1 1 111
User avatar
Firtasik
Member
 
Posts: 450
Joined: 2013-7-21 @ 19:07

Re: 64-bit dynamic_x86 (patch)

Postby Dominus » 2019-10-05 @ 13:03

Thanks for your elaborate correction.
Let's say, in my experience during testing, I found the 32bit build still being faster than the 64bit build. This was on OS X.
User avatar
Dominus
DOSBox Moderator
 
Posts: 8006
Joined: 2002-10-03 @ 09:54
Location: Ludwigsburg

Re: 64-bit dynamic_x86 (patch)

Postby robertmo » 2019-10-05 @ 13:29

i also had 32-bit faster on windows
User avatar
robertmo
l33t
 
Posts: 4792
Joined: 2003-6-18 @ 10:35

Re: 64-bit dynamic_x86 (patch)

Postby jmarsh » 2019-10-05 @ 13:50

You'd have to compare builds made from identical source+with (roughly) the same compiler... the build in the first post is made with VS9 (and missing quite a few optional features), I believe "official" builds are made with mingw.
jmarsh
Member
 
Posts: 317
Joined: 2014-1-04 @ 09:17

Re: 64-bit dynamic_x86 (patch)

Postby digger » 2019-10-05 @ 15:53

robertmo wrote:time for core haxm :)


I would love to see an optional hardware-assisted hypervisor core in DOSBox! :happy: Unfortunately, the DOSBox developers and maintainers have stated that they won't ever be going the virtualization route, for philosophical reasons. It's a shame, though. Having something like that, at least as an option, would be great for playing the later heavier 32-bit protected mode games from the twilight days of the DOS era on current modern-day hardware.

I completely understand the counterargument, though. The x86 architecture may have had a very long run, but its reign won't last forever. We're already seeing a glimpse of a post-x86 future with the accelerated adoption of the ARM, RISC-V and OpenPOWER architectures. With that in mind, only a completely software-based emulator like DOSBox will ensure that we will be able to continue enjoying the old DOS games we love, regardless of what hardware we end up running them on in the coming decades. And those future machines will be more than fast enough to emulate even the most demanding of DOS games at their intended speeds.

In the meantime, what's really missing is a hypervisor that is optimized for games. "VirtualDOSBox", if you will. ;) kjliew's amazing work on implementing Voodoo passthrough in QEMU might be a great starting point for such a solution: viewtopic.php?f=24&t=60950&hilit=kjliew
User avatar
digger
Member
 
Posts: 226
Joined: 2010-2-12 @ 18:15
Location: Amsterdam, the Netherlands

PreviousNext

Return to DOSBox Development

Who is online

Users browsing this forum: Google Feedfetcher and 5 guests