VOGONS


64-bit dynamic_x86 (patch)

Topic actions

Reply 45 of 123, by dreamer_

User metadata
Rank Member
Rank
Member

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-staging/runs/239673808 (you can click on "Summarize warnings" to see the warning count).
after: https://github.com/dreamer/dosbox-staging/runs/242223652

Link to full static analysis report after the merge: https://github.com/dreamer/dosbox-staging/sui … artifacts/69504 (you can find full report for every build in Artifacts in job called "Clang static analyzer").

Attachments

  • Filename
    new-warnings.txt
    File size
    9.5 KiB
    Downloads
    58 downloads
    File license
    Fair use/fair dealing exception

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 46 of 123, by jmarsh

User metadata
Rank Oldbie
Rank
Oldbie

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).

Reply 47 of 123, by dreamer_

User metadata
Rank Member
Rank
Member

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:

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 }

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 48 of 123, by jmarsh

User metadata
Rank Oldbie
Rank
Oldbie
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.

Reply 49 of 123, by dreamer_

User metadata
Rank Member
Rank
Member

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.

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 50 of 123, by jmarsh

User metadata
Rank Oldbie
Rank
Oldbie

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).

Reply 51 of 123, by dreamer_

User metadata
Rank Member
Rank
Member
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:

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:

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 😀

| ← Ceci n'est pas une pipe
dosbox-staging

Reply 52 of 123, by jmarsh

User metadata
Rank Oldbie
Rank
Oldbie

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/daf9e6037596/s … SDL_dx5events.c

Reply 53 of 123, by Kerr Avon

User metadata
Rank Oldbie
Rank
Oldbie

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?

Reply 54 of 123, by Dominus

User metadata
Rank DOSBox Moderator
Rank
DOSBox Moderator

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.

Windows 3.1x guide for DOSBox
60 seconds guide to DOSBox
DOSBox SVN snapshot for macOS (10.4-11.x ppc/intel 32/64bit) notarized for gatekeeper

Reply 56 of 123, by Dominus

User metadata
Rank DOSBox Moderator
Rank
DOSBox Moderator

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.

Windows 3.1x guide for DOSBox
60 seconds guide to DOSBox
DOSBox SVN snapshot for macOS (10.4-11.x ppc/intel 32/64bit) notarized for gatekeeper

Reply 58 of 123, by jmarsh

User metadata
Rank Oldbie
Rank
Oldbie

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.

Reply 59 of 123, by digger

User metadata
Rank Oldbie
Rank
Oldbie
robertmo wrote:

time for core haxm 😀

I would love to see an optional hardware-assisted hypervisor core in DOSBox! 😀 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: Topic 60950