Page MenuHomePhabricator

[PATCH] Fix thread_annotation negtest for thread safety.

Authored by richard.barton.arm on Mar 22 2016, 4:53 AM.



Although not testing the annotations feature, the test still needs guarding for thread-safety otherwise it will not compile at all.

Diff Detail

Event Timeline

richard.barton.arm retitled this revision from to [PATCH] Fix thread_annotation negtest for thread safety..
richard.barton.arm updated this object.
richard.barton.arm added reviewers: EricWF, jamesr.
richard.barton.arm added a subscriber: cfe-commits.
EricWF requested changes to this revision.Mar 22 2016, 10:41 AM
EricWF edited edge metadata.

I think this diff is messed up a little. Could you upload a fixed diff?

This revision now requires changes to proceed.Mar 22 2016, 10:41 AM
richard.barton.arm edited edge metadata.

Sorry - not sure what happened there. That looks better for me.

I don't actually understand this change. This test should compile with or without "-Wthread-safety" warning. This test doesn't actually turn the annotations on, that's the point. If annotations were actually enabled this test would fail to compile.

Could you explain the error your seeing here?

Hi Eric

Sorry for the delay - I originally detected the failure while not running in a totally clean environment with clean sources, but I can reproduce on clean code.

I can reproduce by building with clean clang and libcxx sources:
cmake -DLLVM_PATH=/work/ricbar01/llvm_oss/ -DLIBCXX_CXX_ABI=libcxxabi -DLIBCXX_CXX_ABI_INCLUDE_PATHS=../../libcxxabi_oss/include -DCMAKE_C_COMPILER=/work/ricbar01/llvm_oss/build/bin/clang -DCMAKE_CXX_COMPILER=/work/ricbar01/llvm_oss/build/bin/clang++ -DLIBCXX_ENABLE_THREADS=OFF -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_ABI_LINKER_SCRIPT=OFF ..

I get errors like:
/work/ricbar01/libcxx_oss/test/libcxx/thread/thread.mutex/thread_safety_annotations_not_enabled.pass.cpp:21:8: error: no type named 'mutex' in namespace 'std'

std::mutex m;

From eyeballing the include/mutex it looks like the mutex class is #ifdeffec out when _LIBCPP_HAS_NO_THREADS is unset.

I think the correct fix is "// UNSUPPORTED: libcpp-has-no-threads"

And looking at the current state of the single-threaded test suite this isn't the only test that needs this fix applied.

Thanks for the help Eric. I'm just running a new patch now will put the new patch up when I get back in to office tomorrow.

jamesr edited edge metadata.Mar 23 2016, 12:08 AM

And looking at the current state of the single-threaded test suite this isn't the only test that needs this fix applied.

Indeed! I assumed by reading other nearby tests that there was some mechanism that disabled the thread/ tests when threads were disabled, but didn't verify this. Sorry about the breakage.

richard.barton.arm edited edge metadata.

My local run with no threads works with this updated patch.

EricWF accepted this revision.Mar 23 2016, 1:13 PM
EricWF edited edge metadata.

LGTM! Thanks for the complete patch! The "no-threads" bot should now be green so future breakage will be detected immediately!

This revision is now accepted and ready to land.Mar 23 2016, 1:13 PM

Hi Eric - no problem at all, you practically wrote it for me after all ;-)

EricWF closed this revision.May 6 2016, 10:39 PM

This was committed in r264191.