Compile MuseScore with msvc / Visual Studio

• Apr 7, 2018 - 17:13
Reported version
3.0
Type
Functional
Severity
S5 - Suggestion
Status
closed
Project
Tags

As stated in https://musescore.org/en/node/106066, building MuseScore with Microsoft tools could be interesting. It could give us access to the new QWebEngine (since it's not supported by Qt for Mingw) and maybe give us some more debug tools and speed?

I open this issue to gather some knowledge about MuseScore and MSVC. Here are my notes so far.

  • Deactivate most option (OMR, Jack, Portaudio etc...)
  • In the CMakeLists.txt files, make sure to use WIN32 and MINGW when appropriate
  • in all.h, #define WIN32_LEAN_AND_MEAN to avoid some name clashes with Windows headers
  • Some options recognized by GCC and Clang are not recognized by MSVC (Wextra, Winvalidpch) and need to be changed
  • in zita, make float implicit using 2.0f notation

  • MuseScore uses GCC extension, Clang can cope with them, MSVC can't

    • Variable Length Arrays ot VLAs, very convenient... I don't think we want to stop using this. Any solution to keep this syntax but make it work with msvc?
    • switch, case on a range, easy to workaround, make the code a bit more verbose but workable
  • MSVC doesn't recognize alternative operators "or". || must be used...

  • We can use Q_CC_MSVC to write specific code for MSVC

  • Sleep is not defined with MSCV, but sleep (lower case s) is.
  • dependencies directory contains the dependencies from the zip used for mingw

Comments

Hello, everyone!
I've started working on this port. Status so far:
- Worked through the CMAKE build system files, which are now generating valid projects for MSVC. It's not complete (in particular, I haven't looked yet at the INSTALL target, and I keep finding small changes that need to be made), but now I can start trying to compile, at least.
- WIN32_LEAN_AND_MEAN doesn't seem to be needed. Had to conditionally include a couple of headers for MSVC in all.h, and to #undef 2 macros that were causing problems.
- Now I'm working on ironing out errors and warnings. I'm compiling at warning level 4 (/W4), and trying for a clean build with no warnings.
- The main problem area will be the non-standard VLA (Variable Length Array) extension being used throughout the code. Still pondering what to do with this...
- Current status: subdirs (in MSVC parlance, projects) audiofile, beatroot, diff-match-patch, effects, fluid, kqoauth, ofqf, qtsingleapp and zerberus are compiling clean (0 errors, 0 warnings).
- I'm working on the rest. Will do a PR when I have stabilized it some more (although it will still have errors/warnings..)

Any comments, particularly on the VLA issue, will be appreciated. Some background info/alternatives:
VLA (variable length arrays) are arrays like "int x[n];", where n is NOT a compile-time constant. This is standard-supported in C99, but not in C++ (up to C++17, at least). There has been on-going discussions in the C++ standardization committees about including this or not, and the decision has been not to, due to serious technical reasons (mainly, it opens up the risk of stack overflows, as "n" can be arbitrarily large, and it wreaks havok with the type system, particularly with templates).
Issue is, VLAs are extremely handy in a lot of situations: they are fast, don't require hard-coding limits, and are easy to use. Drawbacks are also huge: being non-standard, they limit the code portability, they create security risks (they could enable buffer overrun attacks), and could lead, in some instances, to difficult-to-debug bugs.
There are several possible alternatives:
- Use fixed-size arrays, with the size the upper bound of the expected values for the VLA size, and checking out-of-bound conditions (difficulties: determining size, what to do if an index is out-of-bounds),
- Use heap-allocated arrays, which can have a run-time defined size, but this could impact program speed, as heap allocation is slower than stack allocation. This could impact some critical code (for example, in the synthesizers),
- Use C++ standard containers instead of arrays. Although these are heap-allocated, it could be possible to pre-allocate them and keep them around to prevent repeatedly allocating and deallocating heap space (this might also work with the previous solution). This alternative would probably have the most impact on the existing code.

Possible replacement for VLAs:
Instead of

int a[n];
 

If timing is not critical:

// Create a vector of the correct size with elements initialized to default value
static std::vector<int> va(n);
// Get a pointer to the first element, to be able to use it as a standard array
int* a = va.data();
 

If timing is critical, and heap allocations are to be minimized:

// Create a static vector, will keep memory allocated
static std::vector<int> va;
// Clean the vector, reserved memory is not released
va.clear();
// Size the vector for the required elements, will be initialized to default value
va.resize(n);
// Get a pointer to the first element, to be able to use it as a standard array
int* a = va.data();
 

Although this is more verbose, it affects only the declaration of the VLA, not the usage. For the rest of the code, after the VLA declaration, nothing changes. This construct has the following characteristics:
- The array is held in the heap, and managed by the std::vector class. std::vector only allocates memory when the reserved memory is not enough for the new array size, and won't release it unless told to explicitly.
- The first version will release the allocated memory when the vector goes out of scope, and do a new allocation each time the code is encountered. This allows for reentrancy, but could have an impact in execution time due to the heap allocation.
- In the second version the vector is declared as static, and won't be deallocated. The reserved memory will be kept from call to call, and there will be no reallocations unless the new requested size is larger than the largest previous allocation. This isn't problematic, unless the function is called recursively (in which case, this approach won't work at all!)
- In the second version, clearing the vector before requesting the required size ensures that the elements are correctly initialized, and that in the case of a re-allocation no copies are performed.
- The C++ standard guarantees that vector elements are contiguous, so that a pointer to the vector's first element will work as a pointer to the first element of an array does (note: this is valid for all element types, except for std::vector<bool>, which is a special case, and where this approach would not work).

Any opinions on this approach?

In reply to by Nicolas

Yes, definitely. Either a macro, or #ifdef/#else/#endif to keep the code as-is for existing targets, and have the alternative for VS. At a later date, if this works well, a new evaluation might be made as to what to use moving forward (like the change from foreach(,) to for(:) recommended in the guidelines for MS3).
That has been my approach so far: keep everything as-is for existing targets, and provide alternative for VS through preprocessor conditionals (except in a couple of cases: I've been changing foreach's to for's for all targets, and I've been renaming local variables whose names where hiding other variables or class members, but both of this changes shouldn't have any impact on the builds).