Page MenuHomePhabricator

[OpenMP] Enable ThreadSanitizer to check OpenMP programs
ClosedPublic

Authored by simoatze on Sep 22 2015, 2:43 PM.

Details

Summary

This patch allows ThreadSanitizer (Tsan) to verify OpenMP programs. It means that no false positive will be reported by Tsan when verifying an OpenMP programs.
This patch introduces annotations within the OpenMP runtime module to provide information about thread synchronization to the Tsan runtime.

In order to enable the Tsan support when building the runtime, you must enable the TSAN_SUPPORT option with the following environment variable:

-DLIBOMP_TSAN_SUPPORT=TRUE

The building will generate a library called libomp_tsan.so.
I guess this is not something that we want, so probably it should be removed, but I will leave it for the first review.

Event Timeline

simoatze updated this revision to Diff 35419.Sep 22 2015, 2:43 PM
simoatze retitled this revision from to [OpenMP] Enable ThreadSanitizer to check OpenMP programs.
simoatze updated this object.
simoatze added a reviewer: hfinkel.
simoatze added a subscriber: openmp-commits.

I have only marked one of the whitespace issues...

runtime/CMakeLists.txt
324 ↗(On Diff #76077)

Could this be moved to kmp_config.h.cmake?

runtime/src/kmp_barrier.cpp
78 ↗(On Diff #76077)

tab instead of 4 spaces

simoatze updated this revision to Diff 35494.Sep 23 2015, 7:09 AM

Fixed tabs to spaces

hfinkel edited edge metadata.Sep 23 2015, 1:42 PM

Thanks for working on this; I'm very excited to see this functionality!

As a general comment, can you always define ANNOTATE_* macros, and just make them empty if TSAN_SUPPORT is not true. What would make things less verbose.

The building will generate a library called libomp_tsan.so.
I guess this is not something that we want, so probably it should be removed, but I will leave it for the first review.

Why would this not be desirable? What's the alternative?

simoatze updated this revision to Diff 35553.Sep 23 2015, 2:00 PM

Activated by default ANNOTATIONS (switch on/off only with TSAN_SUPPORT).

Thanks for working on this; I'm very excited to see this functionality!

As a general comment, can you always define ANNOTATE_* macros, and just make them empty if TSAN_SUPPORT is not true. What would make things less verbose.

The dynamic annotations have a DYNAMIC_ANNOTATIONS_ENABLED macro to define or not the annotations.
I introduced the TSAN_SUPPORT macro to give a better idea that they are related for supporting ThreadSanitizer.
I just modified the patch setting by default DYNAMIC_ANNOTATIONS_ENABLED=1 so the annotations will be enabled by setting TSAN_SUPPORT to TRUE.
Is that what you meant?

The building will generate a library called libomp_tsan.so.
I guess this is not something that we want, so probably it should be removed, but I will leave it for the first review.

Why would this not be desirable? What's the alternative?

Generating the libomp_tsan.so wouldn't require to modify clang to accept a different library name (besides libomp, libgomp and libiomp5)?
I noticed that if I compile with -fopenmp=libomp_tsan it complains because clang does not know libomp_tsan.so.
Maybe the right thing is just to have one library "libomp.so" and the user would decide to activate Tsan support or not at compile time.

Thanks for working on this; I'm very excited to see this functionality!

As a general comment, can you always define ANNOTATE_* macros, and just make them empty if TSAN_SUPPORT is not true. What would make things less verbose.

Oh I guess you mean to remove all the TSAN_SUPPORT and just leave the ANNOTATIONS, if TSAN support is not defined it will make them empty, right?

Thanks for working on this; I'm very excited to see this functionality!

As a general comment, can you always define ANNOTATE_* macros, and just make them empty if TSAN_SUPPORT is not true. What would make things less verbose.

Oh I guess you mean to remove all the TSAN_SUPPORT and just leave the ANNOTATIONS, if TSAN support is not defined it will make them empty, right?

Yes, exactly. That way each annotation is one line instead of three ;)

simoatze updated this revision to Diff 35555.Sep 23 2015, 2:44 PM

Removed TSAN_SUPPORT macro.

jcownie edited edge metadata.Sep 24 2015, 4:52 AM

It generally looks fine to me.

My one concern is over the licence in the header file. It looks like a BSD-ish licence, but it's not the same as either of the licences which apply to the rest of the code ( http://llvm.org/svn/llvm-project/openmp/trunk/LICENSE.txt ). (Of course, IANAL).

Adding Joachim, who is the main creator of the patch.
He can give more details if needed.

The building will generate a library called libomp_tsan.so.
I guess this is not something that we want, so probably it should be removed, but I will leave it for the first review.

Why would this not be desirable? What's the alternative?

I see two ways to work around the duplicate build for the two libraries:
We might add weak, empty copies of the annotation functions to the library which would be overwritten by the tsan-rt functions if these are loaded. This would be at the cost of additional function calls for each annotation.

Keeping the separate instrumented library, we might define the library as an additional build target, so we build both libraries with a single make step. But I found the latest version of the CMake file very confusing for adding a second library target with only slightly different options.

jlpeyton edited edge metadata.Sep 28 2015, 12:19 PM

Keeping the separate instrumented library, we might define the library as an additional build target, so we build both libraries with a single make step. But I found the latest version of the CMake file very confusing for adding a second library target with only slightly different options.

It's probably confusing because it isn't structured to do that. It is somewhat modeled after libcxx's CMake which does something similar (compile just a single library) although ours is always shared. i.e., there is a single, hard-coded add_library(omp SHARED ...) call. Hence, only one library can be built. I know compiler-rt has custom CMake AddClangRuntimeLibrary(${libname} ...) functionality where several slightly different libraries are generated for the compiler. For instance, when I build llvm/clang on x86 about 40 libclang_rt*.a's are created. If we need this functionality it will have to be added to the CMake logic.

It generally looks fine to me.

My one concern is over the licence in the header file. It looks like a BSD-ish licence, but it's not the same as either of the licences which apply to the rest of the code ( http://llvm.org/svn/llvm-project/openmp/trunk/LICENSE.txt ). (Of course, IANAL).

Urf. Speaking as a lawyer, in practice, this doesn't matter.

However, we should fix all of these, not just the Google one.

The BSD license in that project should not be the license we use. We have generally not given credit to specific groups *in the license*, as that LICENSE does, but instead elsewhere. The license should be the same license we use for runtime libraries elsewhere (which is not even BSD, but MIT).

I'll start a thread about this.

I'll start a thread about this.

is this related to http://lists.llvm.org/pipermail/llvm-dev/2015-October/091536.html?

The intention of using this annotation header file was, to make the annotations available not only to TSan, but also to other tools like valgrind/helgrind.

Do you expect us to align the license to the licenses used by LLVM/OpenMP or will someone from your team take care of this?

Do you expect us to align the license to the licenses used by LLVM/OpenMP or
will someone from your team take care of this?

I do not believe that we (the non-license holders) can arbitrarily change the license.
(Otherwise what was the point of a license in the first place!?)
Therefore the license changes have to be made by people submitting the code.

  • Jim

James Cownie <james.h.cownie@intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438

It generally looks fine to me.

My one concern is over the licence in the header file. It looks like a BSD-ish licence, but it's not the same as either of the licences which apply to the rest of the code ( http://llvm.org/svn/llvm-project/openmp/trunk/LICENSE.txt ). (Of course, IANAL).

Urf. Speaking as a lawyer, in practice, this doesn't matter.

However, we should fix all of these, not just the Google one.

The BSD license in that project should not be the license we use. We have generally not given credit to specific groups *in the license*, as that LICENSE does, but instead elsewhere. The license should be the same license we use for runtime libraries elsewhere (which is not even BSD, but MIT).

I'll start a thread about this.

@dberlin , how do we move forward here? Having the OpenMP runtime library work well with TSan is important.

To summarize the e-mail thread which phabricator did not pick up: @dberlin has authorized a change of the license of dynamic_annotations.h file to comply with LLVM's standard licencing policy.

@jlpeyton, @AndreyChurbanov , et al., is this patch otherwise okay to go in?

I agree that we should make the CMake setup more compiler-rt-like, so we can have multiple targets. Generalizing that logic, or otherwise reusing it, seems like something we can/should do as a separate change.

simoatze updated this revision to Diff 74576.Oct 13 2016, 1:37 PM
simoatze edited edge metadata.

We rewrote the annotations file to deal with the license issue.

If configured defining the following variable

-D LIBOMP_TSAN_SUPPORT=TRUE

the annotations will be enabled in the main shared library (same mechanism of OMPT).

jlpeyton added inline comments.Oct 19 2016, 3:11 PM
runtime/CMakeLists.txt
322 ↗(On Diff #76077)

Unused?

323–326 ↗(On Diff #76077)

It seems TSAN is only supported on Linux X86-64 so inside cmake/config-ix.cmake this should be appended:

if("${CMAKE_SYSTEM_NAME}" MATCHES "Linux" AND ${INTEL64})
  set(LIBOMP_HAVE_TSAN_SUPPORT TRUE)
else()
  set(LIBOMP_HAVE_TSAN_SUPPORT FALSE)
endif()

and in the main CMakeLists.txt file (You'll see this pattern a lot in this file):

set(LIBOMP_TSAN_SUPPORT FALSE CACHE BOOL
  "TSAN-support?")
if(LIBOMP_TSAN_SUPPORT AND (NOT LIBOMP_HAVE_TSAN_SUPPORT))
  libomp_error_say("TSAN functionality requested but not available")
endif()

Also, as Jonas pointed out, we usually use the kmp_config.h.cmake file instead of sending in definitions. So in kmp_config.h.cmake there should be:

#cmakedefine01 LIBOMP_TSAN_SUPPORT
#if LIBOMP_TSAN_SUPPORT
#define TSAN_SUPPORT
#endif
runtime/src/kmp_barrier.cpp
305 ↗(On Diff #76077)

I'm not familiar with the thread sanitizer or what these functions do. Can you explain a little bit about BARRIER_BEFORE and AFTER and what they do with their argument?

runtime/src/tsan_annotations.h
20 ↗(On Diff #76077)

#include "kmp_config.h"

simoatze updated this revision to Diff 75503.EditedOct 21 2016, 4:26 PM
simoatze marked 5 inline comments as done.

I updated the patch according to the last comments.
I have tried to compile the runtime on PPC64LE and I was getting several compiling errors, have anybody compiled the runtime on power recently?

simoatze added inline comments.Oct 21 2016, 4:27 PM
runtime/CMakeLists.txt
323–326 ↗(On Diff #76077)

Tsan is also supported for other architectures (I personally ported it on PPC64), however I wasn't able to compile the OpenMP runtime on PPC64, I'll leave only x86_64 for now.

runtime/src/kmp_barrier.cpp
305 ↗(On Diff #76077)

Those annotations basically update the vector clock for a certain thread that Tsan keeps to compute the happens-before relation.

jlpeyton accepted this revision.Oct 26 2016, 10:29 AM
jlpeyton edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 26 2016, 10:29 AM
Hahnfeld requested changes to this revision.Oct 26 2016, 11:38 PM
Hahnfeld added a reviewer: Hahnfeld.

There are also some indention errors (especially when annotating the reductions)...

runtime/src/kmp_barrier.cpp
796–802 ↗(On Diff #76077)

Is that dead code?

runtime/src/kmp_tasking.c
1212–1221 ↗(On Diff #76077)

You are missing braces here, this won't work as expected!

1315–1322 ↗(On Diff #76077)

You are missing braces here, this won't work as expected!

runtime/src/tsan_annotations.h
87–102 ↗(On Diff #76077)

What's the difference between the new and the old barrier annotations? Can the old ones be removed?

This revision now requires changes to proceed.Oct 26 2016, 11:38 PM
simoatze updated this revision to Diff 76077.Oct 27 2016, 11:45 AM
simoatze edited edge metadata.
simoatze marked an inline comment as done.

Fixed indentation errors, removed dead code and old barrier annotations.

Hahnfeld accepted this revision.Oct 27 2016, 11:13 PM
Hahnfeld edited edge metadata.

LGTM now

This revision is now accepted and ready to land.Oct 27 2016, 11:13 PM
This revision was automatically updated to reflect the committed changes.

Are there any benchmarks yet on potential performance penalties from building libomp with -D LIBOMP_TSAN_SUPPORT=TRUE as the release version of the library?

Are there any benchmarks yet on potential performance penalties from building libomp with -D LIBOMP_TSAN_SUPPORT=TRUE as the release version of the library?

Hi Jack,

in the past we used the EPCC microbenchmarks to measure the difference when OMPT is disabled, enabled and enabled with a tool attached. I think this should also work here...

Regards,
Jonas