Page MenuHomePhabricator

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

Authored by abeserminji on Oct 4 2018, 3:53 AM.

Details

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.

Diff Detail

Repository
rT test-suite

Event Timeline

abeserminji created this revision.Oct 4 2018, 3:53 AM

Also, please always publish Phabricator reviews with full context.

SingleSource/UnitTests/C++11/CMakeLists.txt
2–8

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–8

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 reviewers: MatzeB, atanasyan.

Added necessary flags for Mips only.

MatzeB added a comment.Nov 1 2018, 6:10 PM

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
19

What is there TODO here?

MatzeB added inline comments.Nov 1 2018, 6:12 PM
SingleSource/UnitTests/C++11/CMakeLists.txt
3

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

MatzeB added inline comments.Nov 1 2018, 6:16 PM
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.

abeserminji marked 3 inline comments as done.

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.

This revision is now accepted and ready to land.Wed, Jan 23, 6:20 AM
This revision was automatically updated to reflect the committed changes.