Fix MSVC VST compilation #4421

lukas-w · 2018-06-11T15:51:21Z

This PR fixes VST compilation with MSVC (see #3999). Changes made include:

A lot of changes to CMake scripts had to be made. RemoteVstPlugin is now built using ExternalProject_Add when the target platform doesn't match, and is named RemoteVstPlugin32 and RemoteVstPlugin64 for 32bit and 64bit respectively. WineGCC isn't used directly any more, but is passed to the CMake external project with -DCMAKE_CXX_COMPILER=...-
Use GenerateExportHeader instead of the hand-written export.h. The EXPORT define was renamed to LMMS_EXPORT.
Support for vcpkg and numerous other fixes were added by @justnope in Msvc/vst-rebase : msvc support #4352. Thank you and please come back! ❤️
I cherry-picked @Reflexe's SDL2 fixes from Enable sample track recording #3947 to make LMMS usable. Thanks!
RemoteVstPlugin.cpp was ported to use C++11 threads instead of POSIX threads. mingw-std-threads is included as a submodule and used for MinGW versions that don't support C++11 threads natively.
Vestige reads DLL's file headers to determine whether to use RemoteVstPlugin32 or RemoteVstPlugin64. Thanks @tresf for the suggestion.

As a positive side-effect, this enables 64bit VSTs for Linux (fixes #4103). Here's an AppImage: https://transfer.sh/LFwDB/lmms-1.2.0-rc5.137-linux-x86_64.AppImage

This will probably break everything, but it was needed to load 32bit VST plugins with MSVC.

Older versions of MinGW don't provide std::mutex or std::thread

* locale: using path instead of individual files to reduce command line size * remotevstplugin: changed order return type & calling convention (compiler error) * lmmsobj: removed single quotes for command line defines * added vcpkg support & std::make_unique for MSVC * carla: include exports header * package_linux: corrected RemoteVstPlugin name * vstbase: toolchain file conditional on MSVC * Added install for remotevstplugin * msvc: installer works with vcpkg Remotevst 64bit install removed due to an ApImage problem

* Remove trial-and-error approach of detecting VST's machine types. Read PE headers instead. * Add RemoteVstPlugin64 to AppImage

(with backward compatibility for SDL1).

SDL2's.

@PhysSong

The zero index device may not be the default device. Many thanks to @PhysSong.

tresf · 2018-06-11T20:30:03Z

cmake/linux/winegcc_wrapper.in

+#!/bin/sh
+# Wrapper script for winegcc to remove .exe file ending automatically
+# appended by winebuild.
+# Usage: winegcc <args ...>


Your usage says winegcc. I'm sure this was intentional, but more accurately...

# Usage: winegcc_wrapper <args ...> # --- OR --- # Usage: # CONFIGURE_FILE("path/to/winegcc_wrapper.in winegcc_wrapper @ONLY) # SET(WINEGCC "path/to/winegcc_wrapper") # winegcc <args ...>

Probably nitpicking, but didn't know if it was a typo or intentional. :)

This wasn't intentional, I don't even know why I wrote this in the first place. :) Although your version is definitely better, I think I'll replace it with a note to FindWine.cmake since it's not supposed to be used outside of that context anyway.

winegcc_wrapper.in is only intended to be used within FindWine.cmake. Also moved it to the same directory for this reason.

lukas-w · 2018-06-16T11:43:06Z

I'll be merging this shortly if there are no objections. This changes a lot of things in CMake, so I it's possible it breaks the build for some Linux users. Feel free to complain to me if that's the case.

PhysSong · 2018-06-19T00:20:34Z

plugins/vst_base/VstPlugin.cpp

-		m_badDllFormat = false;
-		tryLoad( "32/RemoteVstPlugin32" );
+	case PE::MachineType::amd64:
+		tryLoad( "RemoteVstPlugin64" );


If I understand correctly, this will try to load 64bit DLL in 32bit system using nonexistent RemoteVstPlugin64. It will print an error message, but Windows users can't see that.
Do we need to try this on 32bit LMMS?

This is correct, there's no visible error message in the GUI when loading fails. This PR doesn't change that.

I think error message issue can be fixed later, but do we ever need to try to load VST with RemoteVstPlugin64 on 32bit systems?

Theoretically, we could ship a RemoteVstPlugin64 with our 32bit Windows installers for users who install 32bit LMMS because they're unaware of the difference. In that case, it's good to try RemoteVstPlugin64. In any other case, there's no harm done here. tryLoad will discover that RemoteVstPlugin64 doesn't exist and will abort, at which point we can easily add an error message later.

Got it, I didn't think about that.

I think we may try to detect processor in runtime if possible, though it looks like out of scope.
Qt provides QSysInfo::currentCpuArchitecture as of Qt 5.4, and it's also possible to use OS-dependent detection.

I opened #4447 to track it.

lukas-w · 2018-06-20T19:44:40Z

Anything left to change @PhysSong or should I merge?

PhysSong · 2018-06-20T22:26:48Z

I'll review this shotrly. If it looks fine, you can go ahead.

PhysSong · 2018-06-21T02:01:43Z

cmake/modules/FindWine.cmake

@@ -24,6 +31,10 @@ STRING(REPLACE " " ";" WINEBUILD_FLAGS "${WINEBUILD_OUTPUT}")

 FOREACH(FLAG ${WINEBUILD_FLAGS})
 	IF("${FLAG}" MATCHES "libwinecrt0.a.*")
+		STRING(REGEX REPLACE "/wine/libwinecrt0.a.*" "" FLAG "${FLAG}")
+
+		SET(WINE_64_LIBRARY_DIR "${FLAG}/")


Unusual, but it is possible to use 32-bit WINE toolchain wine32-tools on 64-bit Debian/Ubuntu. In that case, 64-bit VST compilation needs the inverse replacing(32-bit toolchain uses 32-bit libraries, even if -m64 is specified).

Is that something we have supported before and that this PR breaks?

Is that something we have supported

It's an issue when building 64-bit VST on Linux, so no.

PhysSong · 2018-06-21T02:04:06Z

plugins/CMakeLists.txt

 		vst_base
+		vestige


I think there's no reason to move this.

There is, vestige links to vstbase, so vst_base needs to be processed first. Otherwise, the target doesn't exist when vestige tries to link to it.

PhysSong · 2018-06-21T02:04:32Z

include/stdshims.h

@@ -7,8 +7,10 @@
 #include <memory>
 #include <utility>

-#if (__cplusplus >= 201402L)
+#if (__cplusplus >= 201402L || _MSC_VER)


It looks VS 2012 incompatible.
#4352 (comment)

That's fine. VS 2012 doesn't even implement C++11, so we can't support it.

PhysSong · 2018-06-21T02:06:05Z

plugins/vst_base/RemoteVstPlugin.cpp

-		{
-			sendMessage( IdVstBadDllFormat );
-		}
+		DWORD error	= GetLastError();


Minor, but a tab before = doesn't look good here.

PhysSong · 2018-06-21T02:08:03Z

plugins/vst_base/VstPlugin.cpp

-		m_badDllFormat = false;
-		tryLoad( "32/RemoteVstPlugin32" );
+	case PE::MachineType::amd64:
+		tryLoad( "RemoteVstPlugin64" );


I think error message issue can be fixed later, but do we ever need to try to load VST with RemoteVstPlugin64 on 32bit systems?

PhysSong · 2018-06-21T02:10:54Z

src/core/RemotePlugin.cpp

@@ -62,7 +63,10 @@ void ProcessWatcher::run()
 	{
 		fprintf( stderr,
 				"remote plugin died! invalidating now.\n" );
+
+#ifndef SYNC_WITH_SHM_FIFO


Could you explain why did you add this check?

If you look at the commit that added this change (9db8cbf#diff-b6e06d3d61c47ede770ac49ad9ea8f9e), you can see that I removed an invalidate() call guarded by the same check in processFinished. I removed it because it prevented receiving messages after the plugin died (related: #4103 (comment), #4371, 4fd8ecd). I figured @jasp00 had a reason to add the check though and simply missed this invalidate call, because I couldn't see a point in doing the invalidate twice. So I added the checks here because I removed them together with the invalidate call in processFinished. I'm not sure about this though, it's mostly guesswork. Maybe @DomClark or @Wallacoloo have any insight on this?

Since SYNC_WITH_SHM_FIFO is defined on Windows, this check will prevent LMMS from invalidating plugin. There's no other calls to invalidate on Windows, so it may cause crashes/hangs when remote plugin crashes.
Also, dropping the call in processFinished may result in delaying invalidation because it checks process state every 200ms. I guess call was added for immediate invalidation.
I guess we can somehow work around the delay and/or the IPC issue.

Should I remove the check? Re-adding the call in processFinished will render @justnope's/@Wallacoloo's 4fd8ecd useless again.

Should I remove the check?

I think yes. There's no reason to check that here.

Re-adding the call in processFinished (...) 4fd8ecd useless

It does, but can't we work around this? Though it's not necessary for now and might be hard, I don't think it's impossible. I don't want to block merging this because of the call though.

It does, but can't we work around this?

We probably can, but I have no idea how to do it.

Anyway, I'll revert both changes because this PR doesn't depend on them any more since the VstBadDllFormat message was replaced by the PE file detection.

Reverts some changes made in 9db8cbf. The consequences of this changes are unsure, so reverting them for now. Since a VST plugin's architecture is now detected before trying to load it, this fix is not needed any more for 64&32-bit VSTs to work, as the idVstBadDllFormat-message-mechanism was removed. It should be noted however that the bug still exists, probably rendering 4fd8ecd ineffective.

lukas-w · 2018-06-24T16:53:50Z

@PhysSong I fixed the white-space error and removed the invalidate-related changes so they can be discussed in #4371.

Thank you for reviewing! I'll merge once you approve.

PhysSong · 2018-06-25T01:44:36Z

plugins/vst_base/RemoteVstPlugin.cpp

@@ -675,11 +691,11 @@ void RemoteVstPlugin::init( const std::string & _plugin_file )



-static void close_check( int fd )
+static void close_check( FILE* fd )


Minor, but it's not a file descriptor anymore. Might fd be confusing?

PhysSong · 2018-06-25T02:03:16Z

@lukas-w For now, I can't find any more issues. Once you address my comments, I'd say let's merge this.
Anyway, I wonder if other Devs such as @DomClark want to review this one.

lukas-w · 2018-06-25T14:35:27Z

@PhysSong Fixed the potential AppImage problem and the confusing FILE* variable names. Note that the FILE* changes in this PR are in conflict with #4401. Feel free to merge your PR first, I can take care of the merge conflicts then.

# Conflicts: # plugins/vst_base/RemoteVstPlugin.cpp # plugins/vst_base/VstPlugin.cpp

lukas-w · 2018-07-07T17:08:44Z

Rebased to squash some commits and because I failed to attribute ad4c4f0 to @Reflexe and merged via 846a2af.

PhysSong · 2018-07-24T06:20:06Z

plugins/vst_base/VstPlugin.cpp

+	FileInfo(QString filePath)
+		: m_file(filePath)
+	{
+		m_file.open(QFile::ReadOnly);


This file open can fail if the path passed to VstPlugin is relative. So I can't load some VSTs...
There should be a path check logic like one in VstPlugin::tryLoad.

Fixed via 7ddca85

PhysSong · 2018-07-26T06:03:10Z

cmake/modules/FindWine.cmake

@@ -33,12 +44,19 @@ FOREACH(FLAG ${WINEBUILD_FLAGS})
 		# WineHQ (/opt/wine-stable, /opt/wine-devel, /opt/wine-staging)
 		STRING(REPLACE "/lib64/wine/" "/lib/wine/" FLAG "${FLAG}")



This logic doesn't work anymore because we drop /wine/ before it and causes #4501.
Some possible solutions:

Use a regular expression to match trailing lib64

Remove /wine/libwinecrt0.a.* after replacing

Always replace lib64 with lib if it doesn't break other platforms

Thanks for tracking this down. Fixed with 515fefa.

lukas-w and others added 29 commits June 7, 2018 11:08

Use CMake GenerateExportHeader

884601f

CMake: Fix MSVC architecture detection

74d4b2f

MSVC: Port RemoteVstPlugin

81a0ec3

RemoteVstPlugin: Debug LoadLibrary failure

c41d59b

MSVC: Fix VST build

2b1b3d3

VST: Fix main entry calling convention

b416036

This will probably break everything, but it was needed to load 32bit VST plugins with MSVC.

Linux compile fixes

9c35487

Fix Linux VST compilation

c3d0dc5

MSVC: Fix VST arch detection

e95a587

MSVC: Fix RemoteVstPlugin module path

e644202

VST build fixes

8fce500

CMake quoting fixes

f702738

MinGW fixes

5b9579d

Older versions of MinGW don't provide std::mutex or std::thread

Fix export errors with MinGW

9a9580a

Mingw64 compilation fixes

a0bd296

More export fixes

39d83ee

Fix 64bit VSTs on Linux by fixing callback calling convention

be0c02f

CMake: Fix Clang detection

5744c2a

Enable 64bit VSTs on Linux

9db8cbf

* Remove trial-and-error approach of detecting VST's machine types. Read PE headers instead. * Add RemoteVstPlugin64 to AppImage

MSVC VST compilation fixes

cd35ec2

AudioSdl: Add support for full SDL2 with float samples and recording

225e902

(with backward compatibility for SDL1).

AudioSDL -> SDL2: Fix a crash from calling a SDL1 function instead of

aa2c867

SDL2's.

Fix cherry-pick

3d98b0a

AudioSdl: Use NULL for device names in order to get the default device.

ad4c4f0

The zero index device may not be the default device. Many thanks to @PhysSong.

Fix AppImage VST

00fda3f

CircleCI: Display Appimage log when failing

4d5eb7f

Fix AppImage 64bit RemoteVstPlugin libwine discovery

0349b97

VstPlugin: Fix define naming conflict with MinGW

af57300

lukas-w mentioned this pull request Jun 11, 2018

MSVC support #3999

Open

15 tasks

tresf reviewed Jun 11, 2018

View reviewed changes

winegcc_wrapper: Remove misleading usage hint

57fdaed

winegcc_wrapper.in is only intended to be used within FindWine.cmake. Also moved it to the same directory for this reason.

lukas-w force-pushed the msvc/vst branch from d7297f5 to 57fdaed Compare June 16, 2018 11:24

lukas-w mentioned this pull request Jun 17, 2018

MSVC: Library find modules #4438

Merged

PhysSong reviewed Jun 19, 2018

View reviewed changes

PhysSong reviewed Jun 21, 2018

View reviewed changes

lukas-w mentioned this pull request Jun 24, 2018

Add error messages if VST loading failed #4447

Closed

lukas-w added 2 commits June 24, 2018 18:40

Whitespace fix

642703e

PhysSong reviewed Jun 25, 2018

View reviewed changes

lukas-w added 2 commits June 25, 2018 16:33

CircleCI: Make sure build fails when AppImage building does

66c2047

RemoteVstPlugin: Fix confusing variable names

65ccaff

Merge branch 'master' into msvc/vst

d04965a

# Conflicts: # plugins/vst_base/RemoteVstPlugin.cpp # plugins/vst_base/VstPlugin.cpp

lukas-w added a commit to lukas-w/lmms that referenced this pull request Jul 7, 2018

Merge branch 'msvc/vst-rebase' (LMMS#4421)

846a2af

lukas-w closed this Jul 7, 2018

lukas-w deleted the msvc/vst branch July 7, 2018 17:08

PhysSong mentioned this pull request Jul 10, 2018

Target RemoteVstPlugin does not detect dependency changes #3059

Closed

PhysSong reviewed Jul 24, 2018

View reviewed changes

PhysSong reviewed Jul 26, 2018

View reviewed changes

PhysSong mentioned this pull request Aug 21, 2018

Running Cmake warns you about Policy CMP0072 not being set #4499

Open

PhysSong mentioned this pull request Nov 26, 2018

master: Can't load 32bit VST plugins with 64bit Windows version of LMMS due to dll clashes #4717

Closed

PhysSong mentioned this pull request Jul 31, 2019

lmms 1.2.0: text relocation in /usr/lib/lmms/RemoteVstPlugin.exe.so #5024

Open

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Merge branch 'msvc/vst-rebase' (LMMS#4421)

7d648a4

		@@ -675,11 +691,11 @@ void RemoteVstPlugin::init( const std::string & _plugin_file )



		static void close_check( int fd )
		static void close_check( FILE* fd )

		@@ -33,12 +44,19 @@ FOREACH(FLAG ${WINEBUILD_FLAGS})
		# WineHQ (/opt/wine-stable, /opt/wine-devel, /opt/wine-staging)
		STRING(REPLACE "/lib64/wine/" "/lib/wine/" FLAG "${FLAG}")

Fix MSVC VST compilation #4421

Fix MSVC VST compilation #4421

Conversation

lukas-w commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-w commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-w commented Jun 20, 2018

PhysSong commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-w commented Jun 24, 2018

Choose a reason for hiding this comment

PhysSong commented Jun 25, 2018

lukas-w commented Jun 25, 2018

lukas-w commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment