Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mingw gcc-6.2 ICE. #2473

Merged
merged 4 commits into from Feb 13, 2017
Merged

Fix mingw gcc-6.2 ICE. #2473

merged 4 commits into from Feb 13, 2017

Conversation

awson
Copy link
Contributor

@awson awson commented Nov 4, 2016

This fixes mingw gcc-4.9+ ICE

I can now build HEAD supercollider with stock MSYS2 64-bit gcc compiler/libraries (no manual tools/libraries installation).

Perhaps, this should be submitted to boost in the first place, though.

I've also guarded this with __MINGW32__ since I'm not sure if another platform's GCCs suffer from the problem.

@bagong
Copy link
Contributor

bagong commented Nov 4, 2016

Wow, your name should be awesome ;)

@crucialfelix
Copy link
Member

Needs testing. This is for 3.9 ?

@bagong
Copy link
Contributor

bagong commented Nov 4, 2016

Yea, untested so far. Change is in boost, as awson mentioned himself, so it might even need discussion ;) It's not trivial to test, because it requires a fitting build environment.

@crucialfelix crucialfelix added this to the 3.9 milestone Nov 4, 2016
@bagong
Copy link
Contributor

bagong commented Nov 8, 2016

@awson, I tried the build, and it breaks in hidapi for me:

https://gist.github.com/bagong/dd42da9bf990bc11e621eb0a2a53a9f4

Unfortunately I won't be around to follow up on this during the next 2 months and it is therefore possible that you don't get much feedback on this interesting pull request, I am sorry to say. I like msys2, and it provides an interesting alternative for qt, as the msys2 maintainers provide webkit in qt5.6 too - which is not the case in the official Qt distribution. Oth I must also say, that there is a strong trend to move towards Visual Studio among the persons working for the Windows version of SC. So while it would be very cool of course, if you were interested in maintaining a MinGW/msys build, atm it will likely not be an effort with much help by others.

@awson
Copy link
Contributor Author

awson commented Nov 9, 2016

Yeah, a couple of small patches to hidapi (and supercollider proper) is required.
OK, if anybody is interested in stock MSys2 build, I'll put them into.

@bagong
Copy link
Contributor

bagong commented Nov 9, 2016

That would be great!

@awson
Copy link
Contributor Author

awson commented Nov 9, 2016

Hidapi part.

@bagong
Copy link
Contributor

bagong commented Jan 30, 2017

Hi Awsome ( @awson )!

Sorry for taking so long to follow up on this, but I should be ready now if you are still available!

It's getting further and further. I get this error now:

[ 48%] Building CXX object lang/CMakeFiles/libsclang.dir/LangSource/GC.cpp.obj
C:/Users/Rainer/Projects/sc/supercollider/lang/LangSource/GC.cpp: In member function 'bool PyrGC::BlackToWhiteCheck(PyrObject*)':
C:/Users/Rainer/Projects/sc/supercollider/lang/LangSource/GC.cpp:1036:31: error: cast from 'PyrObject*' to 'long unsigned int' loses precision [-fpermissive]
    if (objB && (unsigned long)objB < 100) {
                               ^~~~
make[2]: *** [lang/CMakeFiles/libsclang.dir/build.make:715: lang/CMakeFiles/libsclang.dir/LangSource/GC.cpp.obj] Error 1
make[1]: *** [CMakeFiles/Makefile2:2691: lang/CMakeFiles/libsclang.dir/all] Error 2
make: *** [Makefile:139: all] Error 2

@mossheim
Copy link
Contributor

Oof. The "cast loses precision" complaint used to be a warning, not an error. We should really cleanup things like that. Is there a strictness command-line parameter you can invoke to get around that complaint for now?

@bagong
Copy link
Contributor

bagong commented Jan 30, 2017

@brianlheim : yes, -fpermissive allows the build to get past that point. I ran into a linker error with portaudio in sclang now. Probably that would go if we used the msys2 provided PA. I'll have a look. The msys2 build is interesting for various reasons: a) it's very easy for Linux users to get used to, b) it might allow to run post install tests without major adaptations, c) it provides binary Webkit with Qt 5.6 and d) it's a very active, well maintained project...

@bagong
Copy link
Contributor

bagong commented Jan 30, 2017

Works! ;)

@bagong
Copy link
Contributor

bagong commented Jan 30, 2017

SuperNova gets close, but not quite. It seemed to be happy with installing a special "workaround"-package for dynamic linking in a Windows environment (dlfcn), but then still complained with:

[ 76%] Linking CXX executable Release/supernova.exe
C:/msys/usr/lib/libdl.a(t-d000352.o):fake:(.text+0x2): undefined reference to `__imp_dlclose'
C:/msys/usr/lib/libdl.a(t-d000358.o):fake:(.text+0x2): undefined reference to `__imp_dlopen'
C:/msys/usr/lib/libdl.a(t-d000359.o):fake:(.text+0x2): undefined reference to `__imp_dlsym'
collect2.exe: error: ld returned 1 exit status
make[2]: *** [server/supernova/CMakeFiles/supernova.dir/build.make:107: server/supernova/Release/supernova.exe] Error 1
make[1]: *** [CMakeFiles/Makefile2:3733: server/supernova/CMakeFiles/supernova.dir/all] Error 2
make: *** [Makefile:139: all] Error 2

All in all it nevertheless feels we're close to MinGW/MSYS 64-bit build. I'd rather get that going and switch to msys2 than perpetuate the 'Qt-release-policy-dependant' MinGW build we're using for the 32-bit version atm. I'll look next how 32-bit-sc and sc3-plugins behave.

@bagong
Copy link
Contributor

bagong commented Jan 30, 2017

sc32 and sc3-plugins 32 and 64bit build fine (supernova not tested).

Using msys2 has another advantage: we're currently using portaudio with a hacked build system because our MinGW build requires cmake. The combination mingw/cmake isn't supported by PA. Switching to MSYS2 would allow to use the MSYS2-provided package on the MinGW side and build vanilla PA with Visual Studio - as supported by the PA maintainers. That would simplify SC-Portaudio maintenance significantly.

Building SC with MSYS differs from a linux build only in that you need to specify "MSYS Makefiles" as generator for cmake. If you are used to arch linux, you already know the package manager (it's a pacman clone)...

But supernova needs a bit of work...

@mossheim
Copy link
Contributor

This is great, thank you for this @bagong!

I did some googling ("msys libdl undefined reference __dl_open") and I think you have to add -ldl to your linker options. I'm totally debugging-at-a-distance here, though, so just a suggestion. :)

@vivid-synth
Copy link
Member

@awson thanks for this! Can you also submit this patch to the boost repo (https://github.com/boostorg/thread/blob/develop/include/boost/thread/executors/basic_thread_pool.hpp ) so that we don't get further out of sync w Boost? Thanks!

@awson
Copy link
Contributor Author

awson commented Jan 31, 2017

Well, I've revised my (already old) patches and found an old one missed related to if (objB && (unsigned long)objB < 100) line – we need to patch it to be if (objB && (uintptr_t)objB < 100) (-fpermissive isn't quite a correct solution since it affects things globally).

But I never ever had any problems with supernova – it was built without the hitch. Perhaps -ldl flag was picked by CMAKE automatically during a configuration phase?

@bagong
Copy link
Contributor

bagong commented Jan 31, 2017

Thanks @awson for for staying in the loop. I was not suggesting to introduce -fpermissive globally, I just used to see how far I would get with it. Could you add your patch to this pull request (and maybe others of similar nature ;) )?

I will look into the supernova problem, after all the main reason to keep up a MinGW build is supernova. Prior to the error-message I posted, I had another error claiming that dlfcn.h was missing. I worked around that by installing the package mingw-w64-i686-dlfcn, which in turn produced the error I reported. Should I uninstall the dlfcn package (which I think is controversial) again and go on from there (investigate -ldl) or is dlfcn required for the MSYS2 supernova build?

@@ -49,7 +49,6 @@
#define basename win32_basename
#define dirname win32_dirname
#define pipe win32_pipe
typedef int pid_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this breaks the VS build.

Copy link
Contributor

@bagong bagong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am not in a position to comment on this, I have verified that this fixes a long standing bug in the MinGW compiler line that blocked MinGW builds above gcc 4.82 on Master. We need this, and we cannot wait for boost... (we should keep track of boost changes, though...). I will submit a PR soon, that integrates the MSYS2 option into the SC Windows build system. Thank you very much, Awson!
PS: The supernova errors disappeared, but I did have to install the dlfc package in MSYS2/MinGW...

This means we have a working 64-bit MinGW build now, and the Qt55-Webkit block is removed on the MinGW line. If readline7 were working this would be the first feature complete Windows build including supernova ;)

@mossheim
Copy link
Contributor

mossheim commented Feb 7, 2017

I know I'm coming late to this, but I don't understand why the change had to be made in a Boost header; it's not apparent from the linked Gist. Is there a discussion somewhere else where this was diagnosed?

@awson
Copy link
Contributor Author

awson commented Feb 7, 2017

Hmm, while the offending code is located further down the callstack than the code referred in the gist, the latter is a Boost header's code: C:/Users/Rainer/Projects/sc/supercollider/external_libraries/boost/boost/thread/detail/invoke.hpp:102:43: internal compiler error: in gimplify_expr, at gimplify.c:8343 return (*boost::forward<A0>(a0)).*f.

@mossheim
Copy link
Contributor

mossheim commented Feb 7, 2017

But these warnings/errors are occurring within the context of translation unit PyrObject.cpp... In order to blame this on Boost someone would have to try compiling the Boost header in a less complex situation. This situation looks suspect (lines 43-54 of PyrObject.cpp):

#include <boost/range/irange.hpp> // first boost include

#define BOOST_THREAD_VERSION 4 // define boost marcos without explanation
#define BOOST_THREAD_PROVIDES_EXECUTORS

#include <boost/thread/future.hpp> /* compiler error happens here */
#include <boost/thread/executor.hpp>
#include <boost/thread/executors/basic_thread_pool.hpp>

#ifdef _MSC_VER
#include <future> // ifdef'd include, also for `future`
#endif

@awson
Copy link
Contributor Author

awson commented Feb 7, 2017

Well, this is enough to make gcc ICEing:

#define BOOST_THREAD_VERSION 4
#include <boost/thread/executors/basic_thread_pool.hpp>

void ICE()
{
	boost::basic_thread_pool pool( 4 );
}

Nothing suspicious. Plain and simple gcc bug on purest Boost code.

@mossheim
Copy link
Contributor

mossheim commented Feb 7, 2017

The versions of Boost headers/libraries in this repo span 1.0.45-1.0.61, and chrono, thread, and system are themselves from three close but different versions. So no, not exactly 'purest Boost code'...

Anyway, I'm OK with this change since it's been so thoroughly tested and only affects minGW.

@bagong
Copy link
Contributor

bagong commented Feb 7, 2017

Do we have an organized way of identifying boost patches in SC? I know of two for Windows (including this one). We could keep them somewhere for quick reference if ever a boost reform is done...
I only tested if the builds work, @brian, you know that. If you have a way to test for functionality, let me know. Otherwise I still think this brings us further than we were...

@mossheim
Copy link
Contributor

mossheim commented Feb 7, 2017

I made a note of it in our projects here: https://github.com/supercollider/supercollider/projects/2#card-1643180

I would like somewhere a little more prominent... maybe we should just open an issue for updating the Boost libraries, and then we can collect this information there.

@bagong
Copy link
Contributor

bagong commented Feb 7, 2017

I added the other patch (#2457) I know of.

@awson
Copy link
Contributor Author

awson commented Feb 7, 2017

I've just checked. ICE is here for the latest and greatest stock-single-version-consistent-homogeneous-blessed-don't_know_what_else MSYS2/mingw64 boost-1.63 + gcc-6.3.

@bagong
Copy link
Contributor

bagong commented Feb 7, 2017

It is at the super bowl too! :)
https://www.ice.gov/features/superbowl
And boost for kids has it too:
https://boostforkids.org/programs/internet-child-exploitation/

@mossheim
Copy link
Contributor

mossheim commented Feb 7, 2017

I've just checked. ICE is here for the latest and greatest stock-single-version-consistent-homogeneous-blessed-don't_know_what_else MSYS2/mingw64 boost-1.63 + gcc-6.3.

I really appreciate you checking that, thank you.

Sounds like if we wait long enough, ICE might not be a problem at all:

http://www.telegraph.co.uk/news/2017/01/14/fast-arctic-ice-melting-meet-british-scientist-risked-polar/

@mossheim mossheim merged commit 9207253 into supercollider:master Feb 13, 2017
@mossheim
Copy link
Contributor

Merged after lots of testing documented in #2704.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: build CMake build system os: Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants