#283 Update to INSTALL, fix memory leak and 2 warnings

[Patryk Obara]

Zip file contains 6 patches, grouped by topic; they apply cleanly to ^/dosbox/code-0/dosbox/trunk@4265

Patches were generated via git-format-patch, they are numbered in order of application, each one contains original commit message explaining the patch content.

For easier review you can look them up on GitHub: https://github.com/dreamer/dosbox-staging/compare/po%2Fpatches-for-svn-1~6...po%2Fpatches-for-svn-1?diff=split

[Qbix]

Force C11 support ? When nothing in our current code uses that ?

[Patryk Obara]

C++11, not C11 (these are different standards); every modern compiler uses C++14 as the default C++ standard nowadays - I thought about setting it to C++14, but some really, really, old compilers might not support it (I couldn't find one, but I bet someone would) - I think using 8-year old version of the language is reasonable in 2019. Also, autoconf-archive in Ubuntu 16.04 does not supply the macro for C++14 (compiler supports it, just autoconf packages are outdated there - they support C++11 though - it was 5 year old at the time).

By not setting baseline standard in the code, it is harder to maintain cross-compiler support nowadays. For example: I had my changes working just fine on my local machine using the default compiler for my distro (GCC9) just to discover, that my changes do not compile in older Steam Runtime environment (GCC5.4); and it was not because of lack of support in the compiler - it was because of missing compiler flag enabling said support :).

In this particular patch series, patches 1, 2, 3 do not depend on C++11, while patches 5 and 6 do.

Patch 5: using https://en.cppreference.com/w/cpp/memory/unique_ptr to prevent memory leak, even if later someone will introduce new return statements in this function.

Patch 6: using =default to mark destructor as virtual, without actually introducing empty user-defined constructor in there, thus vtable should stay the same as it is now, but compiler will ensure correct behaviour for subclasses (there is no way to ensure this compiler behaviour pre C++11 AFAIK).

Ah, forgot to mention it with the submission - I intend these changes only for trunk and not to be backported to 0.74 branch.

[Dominik Reichardt]

to cross compile for PPC OS X you have to use ages old gcc 4.0/4.2. C++11 is not supported in that AFAIK.

[Qbix]

My IDE/compiler doesn't support it either.

For patch 6. I don't see why it is needed. There is no deletion of a derived class through a baseclass going on there. The pointer and the class are both saa1099_device.
Anyway, virtual ~device_t(){ } would do the job as well without needing C++11, but again, I fail to see why it is needed. But I'd be fine with the empty destructor.

[Patryk Obara]

Patch 6 addresses following warnings generated by GCC9 (I mentioned this in commit message attached to patch 6):

gameblaster.cpp: In destructor ‘virtual CMS::~CMS()’:
gameblaster.cpp:160:18: warning: deleting object of polymorphic class type ‘saa1099_device’ which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor]
  160 |   delete device[0];
      |                  ^
gameblaster.cpp:161:18: warning: deleting object of polymorphic class type ‘saa1099_device’ which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor]
  161 |   delete device[1];
      |                  ^

If you prefer to have user defined empty destructor in there, it's fine by me - I care about removing these warnings. Should I upload a revised version of this patch?

Re: IDE without C++11 support

Which IDE is it exactly? I am genuinely curious as I couldn't find one. I would ask you to reconsider C++11 support (it makes the language much nicer to use - and you wouldn't need to keep removing C++11 features from code merged from other projects). If it's totally not acceptable, then please state if I should target C++03, C++98 or something older in future patches? Also, please note that Visual Studio does not support enforcing anything older than C++14 at all - so sticking to old standard will be an inconvenience to future contributors, and this topic will keep coming back.

Re: Cross-compilation for PPC OS X

Do you know if current trunk version of DOSBox works on PPC OSX? When was the last time it was tested? I am arguing for using new standard only for DOSBox >= 0.75.x, you would still be able to compile and use 0.74.x branch to emulate games from 25+ years ago on a machine from 15+ years ago. Or you could install Linux on that old machine and use DOSBox 0.75.x just fine.

[Dominik Reichardt]

current trunk works on PPC. Last tested a week ago. I'm not here to fight you on this, just pointing out that it breaks compatibility. Does it need to be broken?

[Qbix]

I read the comment and hence me asking why it generates a warning as there is nothing wrong with the dosbox code. The warning program is being too sensitive and/or too limited to check that the class being deleted matches the polymorphic class. The pointers are even static, but I have to admit that the correct check would quite the nightmare to implement. So it is easier to always give a warning if they see a class with a virtual function, without virtual destructor being deleted. Although maybe the virtual function could change the this pointer. Anyway I am getting of track.

You don't need to submit a new patch for that. I can add an empty constructor myself. But it has no hurry as our code is fine in that instance. I added it to my local tree and it will end up in dosbox, when commit something approriate.

You couldn't find one ? Ha, I am stuck with much older software for various reasons, so think in that direction :)

Anyway, no reason to change to requirements over fixing a memory leak that can't happen, but the analyzer can't figure that out. (fstype can be fat,iso,none (checked above) and when the image is created it is branched through an "else" (which is actually fstype=none as fat and iso are checked before) and the deletion code (that the analyser skips is guarded by fstype==none).

So again the analyser is too limited or our code too messy to follow. :)
I will rewrite it so that the analyser can follow it better (by changing the else to an else if (fstype=="none"). I think that should help the analyser figure it out. (again there is no bug except for maybe being a bit hard to follow)

But maybe I read the code wrong, feel free to point where I messed up.

[Patryk Obara]

@Dominik Reichardt
I think you're talking about this thread, right? https://www.vogons.org/viewtopic.php?f=32&t=65057

Please note krcroft's build from that thread was using Ubuntu 16.04 and GCC 5.4 - my patches were verified on CI using this toolset: https://github.com/dreamer/dosbox-staging/runs/244870391 I will be happy to add cross-compilation for Linux PPC in CI, if it's actually useful (but note, that Ubuntu 16.04 LTS will not be supported by Canonical in ~2 years, so it might be hard to keep it working past that point). As for now, PPC support is causing issues with wav decoding for krcoft's patch: https://github.com/dreamer/dosbox-staging/issues/9, so I'm afraid it might prevent his patches from being merged to trunk (I hope not, because his patch is awesome - it needs some work still, but it improves audio on Linux, fixes mp3 decoding completely, is prerequisite for work on proper SDL2 support and could considerably simplify build process on Windows).

C++11 was huge (and long overdue) update to the language and I would argue that using it is more beneficial to DOSBox project long-term than maintaining macOS X 10.4 PPC support going forward. Is it really a platform you would like to play DOS games on or is support only a retro-computing curiosity? If former, I would argue it's better to switch to Linux - if latter, then I would simply stick to branch 0.74.

[Qbix]

That function is not fun to read though. I will see if I can make it is bit easier to read and follow.
There might be some other bugs or non-clear cases in there.

[Patryk Obara]

@qbix79 I think we posted at the same time :) Static analyzer reported the memory leak for ther "else" branch - most likely it couldn't correlate that else branch is always taken when fstype == "none" (that would require dynamic analysis; I hope value of that string is not taken literally from user input) - I will look at it again tomorrow.

There's no rush with merging these patches to the trunk, take your time to vet them - I can work on my changes on my own Git branch and rebase on top of newer svn/trunk when appropriate.

[Dominik Reichardt]

I'm mostly referring to that I have a buildbot building current SVN and from time to time I actually test it on a PPC machine.
And don't get me wrong I am not fighting for keeping Mac OS X PPC support. Hell, I'm all for switching to SDL2 which will also result in losing PPC support. And I'd welcome that a lot.
but I always wonder about whether it's good to cut off old systems when you don't really need to.

[krcroft]

I see the desire both ways; holding onto SDL 1.2 lets DOSBox support older operating systems - but it's becoming an ever-tightening noose around DOSBox's useability on modern operating systems. I've personally quit using DOSBox on my desktop because of the SDL1 key-drop bug with modern Xorg versions.

The same issues will manifest with coding standard revisions, which are entirely for the benefit of us humans. today's younger programmers (and compilers, books, and stackExchange answers) will converse using modern constructs. We can try only speaking Middle English for the benefit of the few, but may cause more harm than good in a world that speaks contemporary English.

One solution is drawing a line in the sand: "version 0.75 is the last to run on SDL1.2 and C++ spec X" with the subsequent versions running SDL2 and C++ spec-Y, and thus supporting modern OSes.

Dreamer's git workspace would allow both branches to be supported indefinitely; core developers would continue progressing the head branch while maintenance devs would back-port functional fixes into the legacy branch. There is no shortage of talented developers who would eagerly contribute to such an effort if requested.

As for myself, I've been tinkering with this circa-1999 PowerMac G4 (PowerPC-7400 CPU). Besides BSD, Ubuntu 16.04 is the most up-to-date Linux build supporting powerpc. It comes with gcc 5.4.x which has no problems with C++11. And with a bit more work I've recently moved it to gcc-9.2 and kernel 5.3.2.

[Qbix]

went to town and reordered a ton in dos_programs. Should make your static (and somewhat "overly" enthousiastic ) analyzer hopefully a bit more happy. Most warnings fixed were actually false flags, as it didn't understand the whole size parsing code.

[krcroft]

Very nice cleanup!

[Patryk Obara]

Thanks :) These changes pushed number of GCC9 warnings down from 382 to 380 and Clang static analyzer issues from 109 to 100 - you can check details/download static analysis report here: https://github.com/dreamer/dosbox-staging/runs/247088678

Can I get a clarification about the newest allowed C++ standard in DOSBox code? Is C++03 ok?

What about patches 1 and 2?

[Qbix]

C++03 should be fine.

patch 1: While a nice start, I have to add more info to it, as it contains incorrect information. (I changed core-inline to be enabled by default for example), so hence I didn't add it.

patch 2: The error logging that way is a bit weird. I have to double check that piece of code.
(as it doesn't use the translation system as well.) Not sure if possible, but it might be better to return the error to the caller, which would then WriteOut like the rest of the errormessages are done. Not sure though. Got to take a look when I have some time.

With regards to the translation system. More things need to be added to that. I didn't do most of the overlay filesystem messages either.

[Patryk Obara]

I think we can close this issue? Continued in #284

[Qbix]

Now, it can be closed.