Page MenuHomePhabricator

[Clang] Add __STDCPP_THREADS__ to standard predefine macros
ClosedPublic

Authored by zequanwu on Wed, Nov 18, 3:29 PM.

Details

Diff Detail

Event Timeline

zequanwu created this revision.Wed, Nov 18, 3:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 18, 3:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zequanwu requested review of this revision.Wed, Nov 18, 3:29 PM
zequanwu updated this revision to Diff 306561.Thu, Nov 19, 5:07 PM

Add a test case.

rnk added inline comments.Thu, Nov 19, 9:13 PM
clang/include/clang/Frontend/FrontendOptions.h
307 ↗(On Diff #306561)

This doesn't seem to fit with the other frontend options. This seems more like a language option. See the existing PosixThreads langopt.

If you add a langopt, you should be able to remove the option from CodeGenOptions, since codegen can check language options.

zequanwu updated this revision to Diff 306724.Fri, Nov 20, 10:00 AM

Add ThreadModel to LangOptions and remove it from CodegenOption.

rnk added inline comments.Fri, Nov 20, 12:31 PM
clang/test/CXX/cpp/cpp.predefined/p2.cpp
2

Let's expand on this:

  • test that we don't set the macro when compiling C (-x c)
  • test that we don't set the macro when -mthread-model single is passed

Pass an extra macro def on the command line to control the preprocessor test expectations similar to p1.cpp

zequanwu updated this revision to Diff 306777.Fri, Nov 20, 2:04 PM

Update tests.

zequanwu marked an inline comment as done.Fri, Nov 20, 2:05 PM
zequanwu added inline comments.
clang/test/CXX/cpp/cpp.predefined/p2.cpp
2

I found split-file is handy to separate different test expectations.

rnk accepted this revision.Sun, Nov 22, 8:54 AM

lgtm, thanks!

This revision is now accepted and ready to land.Sun, Nov 22, 8:54 AM
This revision was landed with ongoing or failed builds.Sun, Nov 22, 4:06 PM
This revision was automatically updated to reflect the committed changes.

Hi, it seems that p2.cpp is failing on our production builder (https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-linux-x64/b8862835022166171808):

FAIL: Clang :: CXX/cpp/cpp.predefined/p2.cpp (1210 of 26900)
******************** TEST 'Clang :: CXX/cpp/cpp.predefined/p2.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   split-file /b/s/w/ir/x/w/llvm-project/clang/test/CXX/cpp/cpp.predefined/p2.cpp /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/tools/clang/test/CXX/cpp/cpp.predefined/Output/p2.cpp.tmp.dir
: 'RUN: at line 2';   /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/bin/clang -cc1 -internal-isystem /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/clang/12.0.0/include -nostdsysteminc -verify /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/tools/clang/test/CXX/cpp/cpp.predefined/Output/p2.cpp.tmp.dir/defined.cpp
: 'RUN: at line 3';   /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/bin/clang -cc1 -internal-isystem /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/clang/12.0.0/include -nostdsysteminc -verify -mthread-model posix /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/tools/clang/test/CXX/cpp/cpp.predefined/Output/p2.cpp.tmp.dir/defined.cpp
: 'RUN: at line 4';   /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/bin/clang -cc1 -internal-isystem /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/clang/12.0.0/include -nostdsysteminc -verify -mthread-model single /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/tools/clang/test/CXX/cpp/cpp.predefined/Output/p2.cpp.tmp.dir/not-defined.cpp
: 'RUN: at line 5';   /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/bin/clang -cc1 -internal-isystem /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/clang/12.0.0/include -nostdsysteminc -verify -x c /b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/tools/clang/test/CXX/cpp/cpp.predefined/Output/p2.cpp.tmp.dir/not-defined.cpp
--
Exit Code: 127

Command Output (stderr):
--
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/tools/clang/test/CXX/cpp/cpp.predefined/Output/p2.cpp.script: line 1: split-file: command not found

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  Clang :: CXX/cpp/cpp.predefined/p2.cpp

This can be reproduced if you run clang tests before building llvm tools (running check-clang without running check-llvm). Should split-file be used here in the first place since it's an llvm tool? If so, should there be a way to force check-clang to build split-file before running tests?

rnk added a comment.Mon, Nov 23, 12:17 PM

I'll push a fix to add it to the cmake deps list.

In D91747#2412212, @rnk wrote:

Yup, looks like it does. Thanks for the quick fix!

Looks like _LIBCPP_HAS_NO_THREADS is being set for libcxxabi, and the build now fails with this change:

llvm-project/libcxxabi/../libcxx/include/__config:1172:2: error: _LIBCPP_HAS_NO_THREADS cannot be set when STDCPP_THREADS is set

dmajor added a subscriber: dmajor.Mon, Nov 30, 9:05 AM

Looks like _LIBCPP_HAS_NO_THREADS is being set for libcxxabi, and the build now fails with this change:

llvm-project/libcxxabi/../libcxx/include/__config:1172:2: error: _LIBCPP_HAS_NO_THREADS cannot be set when STDCPP_THREADS is set

Indeed: LIBCXX_ENABLE_THREADS is off by default.

@zequanwu, what's your take on this... which part of the system needs to adapt?

rnk added a comment.Mon, Nov 30, 1:23 PM

If we believe the standard says that the compiler is supposed to set __STDCPP_THREADS__, then I think the libc++ #error needs to be adjusted. libcxxabi, or any other client, should be able to define _LIBCPP_HAS_NO_THREADS, and it should work, even if the compiler thinks they are allowed.

In D91747#2423960, @rnk wrote:

If we believe the standard says that the compiler is supposed to set __STDCPP_THREADS__, then I think the libc++ #error needs to be adjusted. libcxxabi, or any other client, should be able to define _LIBCPP_HAS_NO_THREADS, and it should work, even if the compiler thinks they are allowed.

So, we could remove the checking for if __STDCPP_THREADS__ and _LIBCPP_HAS_NO_THREADS are both set. And let libcxx adds flag -mthread-model single to use single thread (but this is compiler specific flag, might need a better solution).

rnk added a comment.Mon, Nov 30, 1:43 PM

So, we could remove the checking for if __STDCPP_THREADS__ and _LIBCPP_HAS_NO_THREADS are both set. And let libcxx adds flag -mthread-model single to use single thread (but this is compiler specific flag, might need a better solution).

Yes, and I think we also need to audit to see if there is any code in libc++ that checks __STDCPP_THREADS__. Actually, I went ahead and did that. It like no other code checks it, so we're probably OK to remove the libc++ check.

It's worth noting that this change will prevent users of older libc++ versions from disabling thread support with new clang. I think that's probably OK.

To connect the dots for posterity: the followup was landed in D92349.