Reply 40 of 123, by jmarsh
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).
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).
Thank you again for the kind help.
So I assume there's no more "illegal option" errors with the latest patch?
time for core haxm 😀
Added in 4260.
Water flows down the stream
How to ask questions the smart way!
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").
| ← Ceci n'est pas une pipe
dosbox-staging
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).
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
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.
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
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).
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 }
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?
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
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
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?
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.
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
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.
i also had 32-bit faster on windows
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.
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