[test-suite] Add flags for stdthreadbug.cpp when building static

[abeserminji]

Summary

This patch adds necessary flags for the test stdthreadbug.cpp, so it can be compiled without error, when tests are compiled with -static flag.

[nemanjai]

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).

[abeserminji]

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?

[abeserminji]

Added necessary flags for Mips only.

[MatzeB]

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?

[MatzeB]

SingleSource/UnitTests/C++11/CMakeLists.txt

3

You can write this as if(ARCH STREQUAL Mips) in cmake.

[MatzeB]

SingleSource/UnitTests/C++11/CMakeLists.txt

3

I meant if(ARCH STREQUAL "Mips")

[atanasyan]

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.

[abeserminji]

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.

[abeserminji]

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.

[atanasyan]

LGTM

SingleSource/UnitTests/C++11/CMakeLists.txt

 list(APPEND CXXFLAGS -std=c++11 -pthread)
-list(APPEND LDFLAGS -lstdc++ -pthread)
+list(APPEND LDFLAGS -lstdc++ -pthread -Wl,--whole-archive -lpthread -Wl,--no-whole-archive)
 llvm_singlesource()

SingleSource/UnitTests/C++11/Makefile

 PROGRAMS_TO_SKIP += stdthreadbug
 endif
 
-LDFLAGS += -lstdc++ -pthread
+LDFLAGS += -lstdc++ -pthread -Wl,--whole-archive -lpthread -Wl,--no-whole-archive
 include $(LEVEL)/SingleSource/Makefile.singlesrc