SFTtech / openage

Free (as in freedom) open source clone of the Age of Empires II engine 🚀
http://openage.dev
Other
12.73k stars 1.12k forks source link

Build failing in OSX 10.10.3 due to issue with thread_local declaration #308

Closed sreejithr closed 9 years ago

sreejithr commented 9 years ago

I did the usual ./configure --mode=debug --compiler=clang and make where it crashes while building thread_id.cpp.

OSX version: 10.10.3 Xcode version: 6.3 Clang version:

Apple LLVM version 6.1.0 (clang-602.0.49) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.3.0
Thread model: posix

Here's the error from make:

[ 31%] Building CXX object cpp/CMakeFiles/openage.dir/unit/unit.cpp.o
[ 32%] Building CXX object cpp/CMakeFiles/openage.dir/unit/unit_container.cpp.o
[ 33%] Building CXX object cpp/CMakeFiles/openage.dir/unit/unit_texture.cpp.o
[ 34%] Building CXX object cpp/CMakeFiles/openage.dir/unit/unit_type.cpp.o
[ 34%] Building CXX object cpp/CMakeFiles/openage.dir/util/thread_id.cpp.o
/Users/sreejith/Code/openage/cpp/util/thread_id.cpp:16:37: error: initializer for
      thread-local variable must be a constant expression
thread_local ThreadIdSupplier const current_thread_id
                                    ^~~~~~~~~~~~~~~~~
/Users/sreejith/Code/openage/cpp/util/thread_id.cpp:16:37: note: use 'thread_local' to
      allow this
/Users/sreejith/Code/openage/cpp/util/thread_id.cpp:16:54: error: expected ';' after top
      level declarator
thread_local ThreadIdSupplier const current_thread_id
                                                     ^
                                                     ;
2 errors generated.
make[3]: *** [cpp/CMakeFiles/openage.dir/util/thread_id.cpp.o] Error 1
make[2]: *** [cpp/CMakeFiles/openage.dir/all] Error 2
make[1]: *** [all] Error 2
make: *** [openage] Error 2

I read that this is a Mac bug with the new Xcode 6.3 update. Folks over at ROOT were having similar issue. Seems they've fixed it (commit).

sreejithr commented 9 years ago

I found this from Clang's implementation status page:

thread_local support currently requires the C++ runtime library from g++-4.8 or later.

Do you guys think it's related to this issue? If yes, would it make sense to use Boost's thread_specific_ptr?

sreejithr commented 9 years ago

GCC is not working btw. Crashes with gcc: error: unrecognized command line option '-Qunused-arguments'. But that's a different issue, I know. I haven't looked into it though. Just brought it up to prove that I'm out options :)

mic-e commented 9 years ago

We have already had issues with thread_local on OSX; cpp/util/compiler.h contains an #ifdef statement that re-defines the thread_local keyword there.

Maybe in the most recent version of XCode, this has become unnecessary/counter-productive? Maybe the preprocessor define didn't trigger for some reason?

sreejithr commented 9 years ago

I don't think the preprocessor is at fault since when I replace thread_local with __thread, I get error: thread-local storage is not supported for the current target. Seems Mac doesn't support TLS.

I found an article about this - Mac OSX Thread Local Storage.

sreejithr commented 9 years ago

For now, I removed thread_local from thread_id.cpp:16 and build was successful. I'm not aware of the consequences.

mic-e commented 9 years ago

The consequence is that thread ids are completely broken, which should mess up thread identification in the logger, and thread handling in the job manager. Your article is from 2010 - that was before even C++11 was released. The addition of thread-local storage to the actual C and C++ standard forced apple to go on and support it. The question is, why has it suddenly stopped working?

sreejithr commented 9 years ago

If it's a problem with the newer Xcode 6.3, I'd have to downgrade it and try out. In the meantime, I installed GNU GCC (since default gcc & g++ in mac is just a symlink to clang) and tried ./configure --mode=debug --compiler=gcc. Doesn't work.

-- The C compiler identification is unknown
-- The CXX compiler identification is unknown
-- Check for working C compiler: /usr/local/bin/gcc
-- Check for working C compiler: /usr/local/bin/gcc -- broken
CMake Error at /usr/local/Cellar/cmake/3.0.2/share/cmake/Modules/CMakeTestCCompiler.cmake:61 (message):
  The C compiler "/usr/local/bin/gcc" is not able to compile a simple test
  program.
mic-e commented 9 years ago

You can always manually specify the full compiler path like so: ./configure --c-compiler /usr/bin/gcc --cpp-compiler /usr/bin/g++

sreejithr commented 9 years ago

The problem with gcc is this gcc: error: unrecognized command line option ‘-Qunused-arguments’.

TheJJ commented 9 years ago

This -Qunused-arguments option is not set by our buildsystem. Do you have any global CXXFLAGS configured somewhere in your work environment?

sreejithr commented 9 years ago

@TheJJ Thanks. You're right. It was set in the environment. The ./configure --mode=debug --compiler=gcc worked. But, on make, it crashed again. I think it tried to compile a C++ file with gcc which is weird.

[  1%] building python modules (via setup.py)
running build_ext
building 'openage.convert.cabextract.lzxd' extension
/usr/local/bin/gcc -Wno-unused-result -Werror=declaration-after-statement -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I/usr/local/include -I/usr/local/opt/sqlite/include -I/usr/local/Cellar/python3/3.4.1_1/Frameworks/Python.framework/Versions/3.4/include/python3.4m -c /Users/sreejith/Code/openage/py/openage/convert/cabextract/lzxd/lzxd.cpp -o build/temp.macosx-10.10-x86_64-3.4/Users/sreejith/Code/openage/py/openage/convert/cabextract/lzxd/lzxd.o -O1 -Wall -Wextra -pedantic -std=c++14 -fdiagnostics-color=auto
cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for C/ObjC but not for C++
In file included from /Users/sreejith/Code/openage/py/openage/convert/cabextract/lzxd/lzxd.cpp:21:0:
/Users/sreejith/Code/openage/py/openage/convert/cabextract/lzxd/lzxd.h:6:18: fatal error: cstdio: No such file or directory
 #include <cstdio>
                  ^
compilation terminated.
error: command '/usr/local/bin/gcc' failed with exit status 1
make[3]: *** [py/timefile] Error 1
make[2]: *** [CMakeFiles/pymodules.dir/all] Error 2
make[1]: *** [all] Error 2
make: *** [openage] Error 2

Now, I've no idea where all this -Wno-unused-result -Werror=declaration-after-statement -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes came from. I didn't see it in the buildsystem. I looked up my env variables, made cmake print out the arguments etc. No clue. You have any idea?

TheJJ commented 9 years ago

These flags come from distutils, the python module we're currently using for building the python extensions. In near future, this will be done differently (see #217).

The problem now is that your compiler doesn't find the c++ headers (namely cstdio, the probably most basic one..). This file is shipped with your C++ standard library, provided by gcc or clang. I fear you have to fix your system once again, this has nothing to do with our project.

sreejithr commented 9 years ago

@TheJJ You were right. I did a fresh reinstall of gcc 5.1.0 and configured the search paths correctly and that problem was solved.

But then, I got error: 'ssize_t' has not been declared. Seems adding a #include <unistd.h> fixed it. Strangely though, clang didn't have a problem with this. Any ideas why?

All in all, I got gcc to compile everything fine :)

TheJJ commented 9 years ago

Nice! What file caused the ssize_t error?

sreejithr commented 9 years ago

@TheJJ It was cpp/console/buf.h.

mic-e commented 9 years ago

As with all standard C++/C symbol names, the standard guarantees that ssize_t is defined in some headers, but it may be available in other, implementation-defined headers.

Thus, if you didn't explicitly include one of the headers that guarantee to provide ssize_t, you might get compiler errors on some combinations of compiler, platform, stdlib and CXXFLAGS.

The proper fix would thus be #include <cstddef>, not #include <unistd.h> (though POSIX may guarantee that unistd.h provides ssize_t as well).

rkreis commented 9 years ago

C++ doesn't know of ssize_t, it's only defined as a part of Posix, which is why <unistd.h> is needed. We should probably use ptrdiff_t instead so we don't require Posix (or: so stuff doesn't break on Windows).