This patch adds necessary flags for the test stdthreadbug.cpp, so it can be compiled without error, when tests are compiled with -static flag.
Details
Diff Detail
Event Timeline
Also, please always publish Phabricator reviews with full context.
SingleSource/UnitTests/C++11/CMakeLists.txt | ||
---|---|---|
2 | As a PPC-Linux guy, I don't really see an issue with this change. However, I am a bit concerned about whether this is portable to other targets. Windows certainly comes to mind - I have no idea how this type of stuff works there (or if test-suite even works there). |
Updated patch to show full context.
SingleSource/UnitTests/C++11/CMakeLists.txt | ||
---|---|---|
2 | I also had this on my mind, so I tested it for windows, and it's a similar behavior as on Mips. Without these flags it's not working well with -static flag. But I am also not sure if this can break this test on some other target. Maybe it would be a better idea if I put these flags only for Mips target? |
I'm not opposed to this change. But out of curiosity:
- Why don't you need to fix the other benchmarks that use -pthread as well?
- If your system requires extra flags to use libpthread, why don't you teach clang to do the right thing when it sees -pthread, I thought that is why we have -pthread in the first place instead of just using -lpthread.
SingleSource/UnitTests/C++11/Makefile | ||
---|---|---|
15 | What is there TODO here? |
SingleSource/UnitTests/C++11/CMakeLists.txt | ||
---|---|---|
3 | You can write this as if(ARCH STREQUAL Mips) in cmake. |
SingleSource/UnitTests/C++11/CMakeLists.txt | ||
---|---|---|
3 | I meant if(ARCH STREQUAL "Mips") |
The problem (if we forget that static linking is usually not a good idea) is not MIPS specific so maybe the following patch solves the problem generally and better?
--- a/lib/Driver/ToolChains/Gnu.cpp +++ b/lib/Driver/ToolChains/Gnu.cpp @@ -513,8 +513,18 @@ void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA, AddRunTimeLibs(ToolChain, D, CmdArgs, Args); - if (WantPthread && !isAndroid) - CmdArgs.push_back("-lpthread"); + if (WantPthread && !isAndroid) { + // Link whole libpthread.a to prevent std::thread + // segmentation fault in case of static linking. + // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52590 + if (Args.hasArg(options::OPT_static)) { + CmdArgs.push_back("--whole-archive"); + CmdArgs.push_back("-lpthread"); + CmdArgs.push_back("--no-whole-archive"); + } else { + CmdArgs.push_back("-lpthread"); + } + } if (Args.hasArg(options::OPT_fsplit_stack)) CmdArgs.push_back("--wrap=pthread_create");
Sure we need to add a test case.
I agree with Simons comment that it's not MIPS specific issue, but in that case I think it would be nice to get comments from other maintainers for other targets.
I didn't see Simons comment and created something similar here D56836, but with some additional checks.
Updated the patch, so that the extra flags are added only when -static option is used.
Used @atanasyan 's solution for CMakeLists.txt from D56836#1362293.
Made equivalent for the Makefile.
As a PPC-Linux guy, I don't really see an issue with this change. However, I am a bit concerned about whether this is portable to other targets. Windows certainly comes to mind - I have no idea how this type of stuff works there (or if test-suite even works there).