vAmiga icon indicating copy to clipboard operation
vAmiga copied to clipboard

Compiling vAmiga on Windows

Open dirkwhoffmann opened this issue 3 years ago • 66 comments

As part of my efforts to improve general code quality, I would like to make vAmiga's code base compatible with Visual C++. Since I don't use Windows regularly, I am unsure about the best approach.

The current situation is as follows: I develop vAmiga on a Mac using Xcode. In addition to the Xcode project file, there are several Makefiles that can be used to compile vAmiga on Linux. There is already a GitHub Action setup that automatically compiles the code in a Ubuntu VM when code is checked in on the main branch.

My first thought was to setup a similar GitHub Action script which uses a Windows VM as target environment. However, I am unsure what I need to do here. I know that Visual C++ comes with nmake. Is it feasible and does it make sense to setup a Windows build environment the same way as I've setup the Ubuntu environment? Or is there another standard approach I am not aware of?

Any input from Window users are welcome.

dirkwhoffmann avatar Nov 18 '21 12:11 dirkwhoffmann

I would recommend using a GH action that uses the default windows image as it has the msvc compiler.

I don't use cmake myself, but it's a quite popular option these days to target many platforms. With cmake you can generate vs compatible files that can be used with compilation.

This is how I setup for building Linux, macOS and windows for my music player. Your setup will differ of course but may still be helpful https://github.com/HippoPlayer/HippoPlayer/blob/master/.github/workflows/main.yml

emoon avatar Nov 18 '21 12:11 emoon

+1 to the recommendation to use CMake. I use that myself for my projects. It generates project files for Visual Studio (and other IDEs) and makes it easy to use another compiler (like Clang on windows for example). Its major downside is the terrible scripting language and somewhat arcane syntax (though it's gotten a bit better over the years), but this shouldn't be an issue for this project, since you're not doing anything complicated / out of the ordinary. On the plus side it's widely used and you can easily find answers to most questions you might have.

I've already tried building with MSVC (using a simple cmakelists file), and it doesn't look too bad, but there are some issues:

  • Use of GCC specific features (not many), but basically anything that starts with double underscore (e.g. __builtin_expect). The builtins are easy to work around, but the use of __attribute__ for alignment won't work (perhaps you can use the standard alignof/alignas as appropriate here).
  • Relying on POSIX stuff (e.g. apra/inet.h, dirent.h, unistd.h and ssize_t). All of these either have direct equivalents, compatibility libraries or even standard replacements (say std::filesystem).
  • Having the file IO.h (from the Utilities directory) in the include path shouldn't be an issue, but it is. On windows (with MSVC and clang) it ends up shadowing a necessary file (that defines some unistd.h replacements, e.g. write).

I also saw, what's probably a MSVC bug (since Clang and GCC accepts it), but I haven't tried debugging it:

1>C:\Prog\amiga\Misc\vAmiga\Emulator\CPU\Moira\MoiraExec_cpp.h(139,1): error C2668: 'moira::Moira::writeM': ambiguous call to overloaded function
1>C:\Prog\amiga\Misc\vAmiga\Emulator\CPU\Moira\MoiraDataflow_cpp.h(318,8): message : could be 'void moira::Moira::writeM<moira::MODE_AN,moira::Word,0>(moira::u32,moira::u32)'
1>C:\Prog\amiga\Misc\vAmiga\Emulator\CPU\Moira\MoiraDataflow_cpp.h(278,8): message : or       'void moira::Moira::writeM<moira::MEM_DATA,moira::Word,0>(moira::u32,moira::u32)'
1>C:\Prog\amiga\Misc\vAmiga\Emulator\CPU\Moira\MoiraExec_cpp.h(139,1): message : while trying to match the argument list '(moira::u32, moira::u32)'
1>C:\Prog\amiga\Misc\vAmiga\Emulator\CPU\Moira\MoiraInit_cpp.h(187): message : see reference to function template instantiation 'void moira::Moira::execAddRgEa<moira::ADD,moira::MODE_AN,moira::Word>(moira::u16)' being compiled

Anyway, none of these issues are insurmountable, and I'll be happy to help out if you have any questions.

mras0 avatar Nov 18 '21 17:11 mras0

@emoon, @mras0: Thank you both for the detailed advice. I will make the switch to CMake.

dirkwhoffmann avatar Nov 19 '21 15:11 dirkwhoffmann

With a bit of hacks (and disabling minor features), I've managed to get latest vAmiga (33e24693aed2ffea21943a1044f59aad815dd353) running on Windows using MinGW (specifically this port)

image

There were a bit more issues that the ones I mentioned in my last post, but again, nothing too bad. Frontend/driver is just a straight forward SDL2 implementation. I think to get it working with MSVC a good first stepping stone is ironing out issues that occur with MinGW, but you decide the course obviously :)

mras0 avatar Nov 22 '21 20:11 mras0

Personally I think it would be good to have it compiling in msvc also. The combo of clang, gcc and msvc usually irons out quite a bunch of general C/C++ "issues" and makes the codebase more portable and compatible.

emoon avatar Nov 22 '21 20:11 emoon

@emoon: 100% agree, but some work (and changes to the emulator core) is required to make that happen.

Add in running cleanly with -fsanitize=address/thread/undefined (I've tried the first one, and basic stuff seemed fine).

Getting to 0 warnings with -Wextra and -pedantic (not to mentioned /W4 with MSVC) will also require extra effort (that may not be worth it, selectively disabling warning is probably appropriate for now).

mras0 avatar Nov 22 '21 20:11 mras0

-pedantic is not worth it. It contains many "compiler internal" warnings that is usually not worth fixing -Wall, -Wextra and msvc /W4 is totally worth it in my book at least :)

I also ran into this the other day. https://lgtm.com I haven't tested it myself, but it's free for open source projects and I will give it a go for sure.

emoon avatar Nov 22 '21 20:11 emoon

I agree. The goal is a code base that

  • compiles with clang, gcc, and msvc.
  • issues no warning with -Wall, -Wextra, /W4.
  • runs cleanly with -fsanitize=address/thread/undefined.

The lgtm project looks very interesting. I haven't heard about it yet.

dirkwhoffmann avatar Nov 23 '21 06:11 dirkwhoffmann

I've set up an experimental cmake branch: https://github.com/dirkwhoffmann/vAMIGA/tree/cmake

As a starting point, I've added two CMakeLists.txt files, one in Emulator and the other one in Emulator/Utilities. There is also a new Dummy.cpp file to make the (cropped) project compile.

The top-level file looks as follows:

cmake_minimum_required(VERSION 3.22 FATAL_ERROR)

# Set project name
project(vAmiga)

# Add include paths
include_directories(${CMAKE_SOURCE_DIR}/..)

# Specify the C++ standard
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED True)

# List all source files
set(files Dummy.cpp)
add_executable(vAmiga ${files})

# Add sub directories
add_subdirectory(Utilities)

target_link_libraries(vAmiga Utilities)

The other one looks looks like so:

set(files
  Chrono.cpp
  Concurrency.cpp
  Exception.cpp
  SSEUtils.cpp
  MemUtils.cpp
  Checksum.cpp
  IO.cpp
  Parser.cpp
)

add_library(Utilities ${files})

When running cmake followed by make, I get:

hoff@Dirks-MBP-2 vAmigaBuild % make clean; make        
[  9%] Building CXX object Utilities/CMakeFiles/Utilities.dir/Chrono.cpp.o
[ 18%] Building CXX object Utilities/CMakeFiles/Utilities.dir/Concurrency.cpp.o
[ 27%] Building CXX object Utilities/CMakeFiles/Utilities.dir/Exception.cpp.o
[ 36%] Building CXX object Utilities/CMakeFiles/Utilities.dir/SSEUtils.cpp.o
[ 45%] Building CXX object Utilities/CMakeFiles/Utilities.dir/MemUtils.cpp.o
[ 54%] Building CXX object Utilities/CMakeFiles/Utilities.dir/Checksum.cpp.o
[ 63%] Building CXX object Utilities/CMakeFiles/Utilities.dir/IO.cpp.o
[ 72%] Building CXX object Utilities/CMakeFiles/Utilities.dir/Parser.cpp.o
[ 81%] Linking CXX static library libUtilities.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libUtilities.a(Exception.cpp.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libUtilities.a(Exception.cpp.o) has no symbols
[ 81%] Built target Utilities
[ 90%] Building CXX object CMakeFiles/vAmiga.dir/Dummy.cpp.o
[100%] Linking CXX executable vAmiga
[100%] Built target vAmiga

Am I on the right track? My current setup will produce a lib file (.a) for all sub directories. Is this the recommended way to go?

dirkwhoffmann avatar Nov 23 '21 17:11 dirkwhoffmann

I won't die defending -pedantic :), I usually just enable it for my own projects (because it's easier if you enable stuff like that early on, like with all warnings). It doesn't matter for code quality only potential portability issues.

Hadn't heard about lgtm before, thanks for the pointer. It was super easy to get working, took me literally 2 minutes and zero effort besides from clicking a few buttons. On the plus side it seems to not give any false positives and isn't "noisy", on the downside it didn't find anything interesting. Because it's apparently possible to enable for any github project, I also tried for vAmiga, but it couldn't deduce how to build it.

I think MSVC's /analyze (that I forgot to mention) finds a lot more useful stuff, the downside is that it's (way) more noisy.

Coverity also offers free scanning of open source projects, and it's (I conjecture) way more advanced that lgtm, but it's harder to setup and get working (at least locally). If the code isn't prepared for it, you also have to dig through many false positives, but I've seen it find very real errors that had been in code bases for a long time and slipped through code review.

mras0 avatar Nov 23 '21 17:11 mras0

Am I on the right track? My current setup will produce a lib file (.a) for all sub directories. Is this the recommended way to go?

Yes, that's totally fine, and probably the most "proper" way to do it. However, if you just want to get something working quickly, you can also just create one big library containing the emulator. The advantage of splitting is up is that you get a better grip of dependencies between the different libraries (modules) and potentially faster compile times when you make changes. The downside is you have to manage dependencies to get it working.

You can see what I've used here:

vAmigaSDL.zip

Note: Using builtin CMake features (like you're doing) is preferred over hacking compiler flags directly, I just do that out of old (bad) habbits. Also I'm building from a sibling directory because I don't want to pollute my "vAmiga" dir.

mras0 avatar Nov 23 '21 17:11 mras0

I won't die defending -pedantic :), I usually just enable it for my own projects (because it's easier if you enable stuff like that early on, like with all warnings). It doesn't matter for code quality only potential portability issues.

So the problem I ran into when doing this is that when you have other people trying to use your code. If they don't have exact matching compiler version it may report different errors. And in cases when you disable some warnings it won't work on compiler x.y because it doesn't know about it.

What I usually end up doing is try with -pedantic and then enable a bunch of extra warnings that isn't included with extra. all as middle ground.

emoon avatar Nov 23 '21 18:11 emoon

You can see what I've used here: vAmigaSDL.zip

Great! This helps a lot.

I've also seen that I can use the target_sources command which makes my CMake files even simpler. I'll try to setup all CMakeLists that way and compile the project locally. Once this is done, I'll add proper GitHub Actions for performing automatic builds.

dirkwhoffmann avatar Nov 23 '21 18:11 dirkwhoffmann

Update:

  • All Makefiles have been replaced by CMakeLists.txt files.
  • There are two GitHub actions now: One for Ubuntu and one for Windows.

As expected, the Windows build fails.

dirkwhoffmann avatar Nov 24 '21 16:11 dirkwhoffmann

I've narrowed down the only issue (the one I mentioned above) that can't be fixed by simple #ifdef-ing code out/providing replacements.

It boils down to MSVC not accepting the following (see on compiler explorer):

enum M { ma };
enum N { na };
struct Foo {
    template<M m> static void f() {}
    template<N n> static void f() {}
};
int main() {
    Foo::f<ma>();
    Foo::f<na>();
}

As I wrote, it's likely a MSVC bug, but it's been too long since I've done any C++ language lawyering to say for certain. I couldn't find any bug reports or similar though.

mras0 avatar Nov 24 '21 18:11 mras0

I asked about the above and someone posted this which was a bit interesting https://godbolt.org/z/ohd9GnoYo now one can see that MSVC is "loosing" the enum type along the way (and only uses the underlying int type)

When asking about how to solve this, the consensus seems to be "Don write code like this".

emoon avatar Nov 25 '21 11:11 emoon

I am basically stuck at the moment, because I cannot install Visual Studio without errors in my VM. Running all this Microsoft typical "Click here to fix me, run this to repair me" stuff seems to make everything worse. I will trash my existing VM and reinstall Windows from scratch.

dirkwhoffmann avatar Nov 25 '21 14:11 dirkwhoffmann

Should work fine from a VM. I have done it a few times in the past.

emoon avatar Nov 25 '21 14:11 emoon

Here's a crude patch (for a2f681fe533e345319b43803597a94b24038a803) that at least gets things compiling: crude_patch.zip. Don't actually include it, but it should highlight where there are issues, and some way to work around some of them.

I can get this far (kick 1.3, no disk inserted) before crashing with those: image

You'll need some kind of executable to actually debug problems. Probably at least a headless version of vAmiga (so you can use retroshell scripts to get e.g. the boot image to see the MSVC version is still working), but a simple SDL gui will probably also help.

I probably broke something with my changes, though it still seems to work under Linux, so take it FWIW.

mras0 avatar Nov 25 '21 19:11 mras0

I've wiped out my VM and reinstalled Windows. This time, I was able to install Visual Studio without errors. However, cmake complains about a non-matching generator. I'm unsure what is meant by "used previously". cmake was running for the very first time on a fresh Windows installation. Does anybody know how to deal with this?

1> [CMake] CMake Error: Error: generator : Ninja
1> [CMake] Does not match the generator used previously: Visual Studio 17 2022
1> [CMake] Either remove the CMakeCache.txt file and CMakeFiles directory or choose a different binary directory.
1> Fehler bei der Ausführung von "C:\WINDOWS\system32\cmd.exe /c "%SYSTEMROOT%\System32\chcp.com 

dirkwhoffmann avatar Nov 25 '21 21:11 dirkwhoffmann

It seems weird that it wants to use the "ninja" generator.

You should be able to select something like "x64 native tools command prompt for VS 2019" from the start menu (press whatever maps to the windows key and type "2019" and it should show up).

Make a directory somewhere convenient and cd into it, the run cmake \path\to\vAmiga should now do something sane (latest visual studio ships with its own CMake now, but only a v3.20 variant, so you may need to change your CMakeLists.txt file like in my patch).

If some of the above fails you maybe didn't get all the right components installed. In that case launch "visual studio installer" and make sure "Desktop development with C++" is enabled".

mras0 avatar Nov 25 '21 22:11 mras0

x64 native tools command prompt for VS 2019

This did the trick. Previously I was running CMake from the developer terminal that can be opened from within Visual Studio.

C:\Users\Dirk Hoffmann\source\repos\vAmiga\EmuBuild>cmake ../Emulator
-- Building for: Visual Studio 17 2022
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.18363.
-- The C compiler identification is MSVC 19.30.30705.0
-- The CXX compiler identification is MSVC 19.30.30705.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.30.30705/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.30.30705/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: C:/Users/Dirk Hoffmann/source/repos/vAmiga/EmuBuild

dirkwhoffmann avatar Nov 25 '21 22:11 dirkwhoffmann

Great! I just noticed you're using a newer version than me (2022 vs 2019), so there might have been some changes (using the developer prompt should have worked). Maybe they even fixed the enum issue. I'll give that a shot tomorrow. Either way the project/solution you got should hopefully work.

mras0 avatar Nov 25 '21 22:11 mras0

I've startet to analyse the MSVC error output. The first error is related to ssize_t which doesn't exist on Windows. I've mapped my custom integer type isize to ssize_t, because I wanted isize to be a "performance type" of the target machine.

@mras0: In your build, you've mapped ssize_tto ptrdiff_t which I think is a good workaround. In fact, using ssize_t might not be a good idea at all as the standard only assures that ssize_t is capable of storing the negative number -1. We don't have the guarantee that it can store -2, although I doubt that there is any architecture out there with this limitation.

What I want to achieve with isize is exactly what int_fast32_t is doing (it provides a type with at least 32 bit and the additional property that no other type with at least 32 bit can be processed faster). Unfortunately, mapping isize to int_fast32_t has drawbacks. E.g., on macOS, this type is mapped to int (4 bytes). On compiler explorer, I got it mapped to long (8 bytes). This means that I would get into trouble with all my printf statements. With the current mapping (to ssize_t), I can use %zd as placeholder, but this would no longer work with int_fast32_t.

dirkwhoffmann avatar Nov 26 '21 07:11 dirkwhoffmann

The solution to the printing problem is easy, but ugly. You would have to use the "Format macro constants" from cinttypes. Extra ugly in C++ because of the note: "Because C++ interprets a character immediately following a string literal as a user-defined string literal,"

mras0 avatar Nov 26 '21 07:11 mras0

easy, but ugly.

std::printf("%+" PRIdFAST32 "\n", 42);

Seriously? No way that I integrate such code 😂.

A nice solution would be to use the C++20 format library instead of printf. https://www.youtube.com/watch?v=KeS1ehp9IiI

Unfortunately, it seems like most compilers still don't support it.

Maybe it's best to not overcomplicate things here and simply map isize to long and usize to unsigned long. This should give us the desired result on all architectures that are relevant for this project.

dirkwhoffmann avatar Nov 26 '21 08:11 dirkwhoffmann

Personally I always do this in my projects

#include <stdint.h>

typedef uint32_t u32;
typedef uint64_t u64;
typedef int8_t i8;
typedef float f32;
typedef double f64;
...

Etc. Just make everything nice, short and to the point imo.

emoon avatar Nov 26 '21 09:11 emoon

Just make everything nice, short and to the point imo.

Yes, I agree. In my opinion, simplicity is the number one rule in software engineering. However, there is a reason why I defined isize with varying width. On macOS, I want this type to map to 64 bit, because of Swift. In this case, isize directly maps to the standard Swift integer type (Swift is so strict about types that it's sometimes a nightmare). But on architectures such as the Raspberry, I want it to be 32 bit because of performance reasons. The same holds for WASM which is still ILP32 (I think).

dirkwhoffmann avatar Nov 26 '21 09:11 dirkwhoffmann

Yeah, I know the arguments for isize as iterator and such because of platform size difference. The thing is that I have seen very little it terms of better performance due to it, so I prefer to have the exact size as it shows the intention better to the reader imo.

emoon avatar Nov 26 '21 09:11 emoon

easy, but ugly.

std::printf("%+" PRIdFAST32 "\n", 42);

Seriously? No way that I integrate such code 😂.

Yeah, this is one place where iostreams (with some helper stuff for formatting) really win out.

A nice solution would be to use the C++20 format library instead of printf. https://www.youtube.com/watch?v=KeS1ehp9IiI

Unfortunately, it seems like most compilers still don't support it.

You could consider using https://github.com/fmtlib/fmt I think it was the testbed for the standard library.

Maybe it's best to not overcomplicate things here and simply map isize to long and usize to unsigned long. This should give us the desired result on all architectures that are relevant for this project.

Just be aware that long is always 32-bits on windows (even 64-bit windows), so they're usually the wrong choice IMHO. If I'm sticking to standard types I use "int" when I want 32-bits and "long long" if I want 64-bits (yes, I know this is not guaranteed, but that can be checked with a static_assert).

But like emoon I usually use the standard types with exact sizes and "size_t/ptrdiff_t" for dealing with containers.

mras0 avatar Nov 26 '21 09:11 mras0