This is an archive of the discontinued LLVM Phabricator instance.

[Tsan] Fix the thread_name tests to build on FreeBSD
ClosedPublic

Authored by kutuzov.viktor.84 on Oct 18 2014, 2:37 AM.

Diff Detail

Event Timeline

kutuzov.viktor.84 retitled this revision from to [Tsan] Fix the thread_name tests to build on FreeBSD.
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added reviewers: kcc, samsonov, glider.
kutuzov.viktor.84 added subscribers: Unknown Object (MLST), emaste.

+dmitry

Dmitry, PTAL at this patch. Also, do we have the list of annotations documented or described anywhere? Shouldn't we make a public header containing them at some point? Forcing users to declare them with "extern "C"" is ugly.

lib/tsan/rtl/tsan_interface_ann.cc
425 ↗(On Diff #15115)

Don't you need to use another name here?

test/tsan/thread_name.cc
7

Shouldn't it be __GLIBC_PREREQ(2, 12) ?

FreeBSD supports setting thread names via (from pthread_np.h):

void pthread_set_name_np(pthread_t tid, const char *name);
test/tsan/thread_name.cc
29

The FreeBSD equivalent is (in pthread_np.h):

void pthread_set_name_np(pthread_t tid, const char *name);
dvyukov added inline comments.Oct 21 2014, 12:28 AM
lib/tsan/rtl/tsan_interface_ann.cc
423 ↗(On Diff #15115)

Please don't add new annotations. They are supported only for historical reasons. A test is definitely not a reason to add an annotation.

Emaste says there is pthread_set_name_np on freebsd; if it's so, use it in the test and intercept it in tsan_interceptors.cc. Otherwise disable the test on freebsd.
Or just disable the test on freebsd. Thread names is not critical functionality.

dvyukov edited edge metadata.Oct 21 2014, 12:36 AM

Also, do we have the list of annotations documented or described anywhere? Shouldn't we make a public header containing them at some point? Forcing users to declare them with "extern "C"" is ugly.

Please don't make annotations any more convenient to use. They are ugly and they should feel ugly.

Emaste says there is pthread_set_name_np on freebsd; if it's so, use it in the test and intercept it in tsan_interceptors.cc.

pthread_set_name_np has existed on FreeBSD since 1998 so there's no need for a version check either.

test/tsan/thread_name.cc
18

BTW, shouldn't it be defined(__linux__) && ...?

BTW, shouldn't it be defined(linux) && ...?

Currently C/C++ tsan works only on linux, so we don't use defined(linux) everywhere.

kutuzov.viktor.84 edited edge metadata.

Updated to use pthread_set_name_np() in place of annotation; relies on D5932.

dvyukov accepted this revision.Oct 23 2014, 5:21 AM
dvyukov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 23 2014, 5:21 AM
Diffusion closed this revision.Oct 24 2014, 2:38 AM
Diffusion updated this revision to Diff 15385.

Closed by commit rL220552 (authored by vkutuzov).