Conversation
|
Could you add a warning to Here's the relevant code: |
That makes sense, but I'd rather not hard-code this conditional twice, so I'll have to refactor some more. |
|
Refactor is done, but I'll have to see what the warnings look like on an machine with an older wine version... Will test and report back... |
|
|
@messmerd since this crash has only ever affected 64-bit VSTs, I'm inclined to show the warning only for 64-bit VST status. Opinions welcome. |
| if(WINE_VERSION VERSION_LESS 8.17) | ||
| # Wine versions prior to 8.17 did not use ASLR by default. Assume off. | ||
| set(WINE_ASLR_ENABLED false) | ||
| elseif(WINE_VERSION VERSION_GREATER_EQUAL 10.14 AND NOT WINE_WANT_ASLR) |
There was a problem hiding this comment.
Is there a situation where someone would ever want to use -DWINE_WANT_ASLR=1?
There was a problem hiding this comment.
Perhaps in the distant future. ASLR as a standard in LMMS should eventually be enabled everywhere (as it has been in all of our other .exe files)... but we just-so-happen to support older VST2s which predate this ASLR.
- If we decide to offer VST3 support through a similar winebridge, it may benefit by having ASLR enabled.
- As a standalone cmake module, this may be desired. I don't know how useful our
FindWineis to others, but the variouswinegcc|wineg++|winebuildatopwine (repackaged)|winehq-stable|winehq-devel|winehq-stagingtricks certainly could be helpful, as well as the internal documentation as to the history of this hidden feature, especially considering that-Wb,isn't even part ofwinegcc --helpoutput... but instead baked into the manpage ofman wine(sincewinegccis just a wrapper, it just spits out thegccflags when asked for help).
There was a problem hiding this comment.
... so in summary, probably not for VST2s, but this is something we may eventually want to make modular for future wine bridges, such as VST3s as well as something that may be desirable if this FindWine.cmake is ever to be adopted officially by cmake or forked by another project that has less restrictions around ASLR handling.
CMakeLists.txt
Outdated
| if(WINE_ASLR_ENABLED) | ||
| set(STATUS_VST_${bits} "${STATUS_VST_${bits}} \n\tWARNING: ASLR is enabled, which may cause crashes with older VSTs") | ||
| endif() |
There was a problem hiding this comment.
Why not print the warning like this?
* VST plugin host : OK, Wine ${WINE_VERSION} cannot disable ASLR - crashes may occur with older Windows VSTs
* 32-bit Windows host : OK
* 64-bit Windows host : OK
There was a problem hiding this comment.
This is only scoped to the 64-bit Windows host (It has nothing to do with the Native Linux VST and IMO doesn't belong there). With regards to the version in the output, that's fine, but I don't believe it belongs in the newly proposed location.
There was a problem hiding this comment.
Good point.
This is tangentially related, but there's a lack of any explicit message about the LinuxVST support, and while we're improving these status messages, maybe that should be fixed too. I'm thinking something like this:
* VST plugin host : OK
* 32-bit Windows host : OK
* 64-bit Windows host : OK
WARNING: You are using wine ${WINE_VERSION} which cannot disable ASLR, please consider upgrading. ASLR may cause crashes with older VSTs.
* LinuxVST host : OK
On Windows, it might look like this:
* VST plugin host : OK
* 32-bit Windows host : OK
* 64-bit Windows host : OK
* LinuxVST host : <not supported on this platform>
The first line ("VST plugin host") would just read "OK" when the Vestige and VST Effects plugins are built.
There was a problem hiding this comment.
Good point.
This is tangentially related, but there's a lack of any explicit message about the LinuxVST support, and while we're improving these status messages, maybe that should be fixed too. I'm thinking something like this:
I agree however I don't particularly care for it even calling it "LinuxVST", since a small expansion of functionality should allow native VSTs on other platforms such as Unix or macOS. Furthemore, I personally find the deliberate mixing of terms for native-compilation (64-bit windows) with cross-compilation (64-bit windows) to be quite confusing and off-putting. Lastly, I don't particularly like the ${bits}|macro looping in our main CMakeLists.txt. I tried to refactor this out a while ago in #7687 but ended up reverting it to make it more mergable.
So, with regards to LinuxVST (or whatever we call it), I'd prefer to tackle this a bit more planned at some future time, since it's completely unrelated to any wine shenanigans.
There was a problem hiding this comment.
... that said, feel free to add something into this PR if you feel strongly about it.
Co-authored-by: Dalton Messmer <messmer.dalton@gmail.com>
Since this wasn't answered, I guess I'll just do it. |
Co-authored-by: Dalton Messmer <messmer.dalton@gmail.com>
Remove custom winebuild task; leverage wine 10.14+ directly instead --------- Co-authored-by: Dalton Messmer <messmer.dalton@gmail.com>
2 participants
Leveraging
winehq-devel(10.14+) directly, which contains the patchedwinebuildneeded to disable ASLR in our AppImages.Closes #8076
Related #7976 #7830 #7987