Page MenuHomePhabricator

[OMPT] Add implementation and tests of Archer tool
Needs ReviewPublic

Authored by protze.joachim on Apr 20 2018, 10:01 AM.

Details

Summary

The tool provides TSAN annotations for OpenMP synchronization. Enabled for
application execution by setting OMP_TOOL_LIBRARIES=libarcher.so, this
library should get installed in the same order as libomp, so the linker
should find the file when the application is linked with OpenMP.

The tool detects whether the application was built with TSan and rejects
activation according to the OMPT protocol if there is no TSan-rt.

Further it is recommended to set
TSAN_OPTIONS="ignore_noninstrumented_modules=1"
This avoids some false positive errors from the OpenMP runtime.

Diff Detail

Event Timeline

Do we really want to call this tool Archer? I understand that it was written for that purpose but IMO this can serve a much more general purpose, even without the Archer "framework" around it.

Reasons to keep the name archer (or archer-rt):

  • We already have that name, we don't need to come up with a new name :)
  • You can easily find related publications under that name, which explain the reasoning behind the library
  • The programmer should never see the name, once the tool is integrated into the runtime workflow

This looks great, thanks guys!
I agree, the name is mostly a "marketing" thing at this point, people know it in that way, changing it would be confusing.

protze.joachim planned changes to this revision.Dec 11 2018, 9:15 AM

This will not compile with the current version of the OMPT interface, needs updates first

The code is updated to be compatible with OMPT as in OpenMP 5.0.

The OpenMP runtime code is modified to automatically load Archer, which will itself only make active if the application is compiled with TSan.

We are currently in the process to relicense the code under "Apache License v2.0 with LLVM Exceptions", but this should have no influence to the code review.

Relicensed Archer under Apache license, updated the files.

Some high-level comments inline

runtime/src/ompt-general.cpp
233–235

These preprocessor guards are redundant.

240–245

If it's not tested, it should be removed.

tools/CMakeLists.txt
8

Why do we need to handle this dummy?

tools/archer/CMakeLists.txt
2–5

I don't think this should be here...

tools/archer/LICENSE.txt
1–3 ↗(On Diff #200309)

there is a LICENSE.txt in the openmp folder, I don't think we should have a copy in this subdirectory.

tools/archer/counter.h
2 ↗(On Diff #200309)

counter.h

tools/archer/ftsan.c
2 ↗(On Diff #200309)

Why do we need this in here? This should surely be handled in the compiler?

protze.joachim marked 5 inline comments as done.

Addressing Jonas' comments

tools/archer/ftsan.c
2 ↗(On Diff #200309)

This allows to use those basic annotations of synchronization in Fortran code.
The annotation interface is also intended for manual instrumenting application code.

Like for SPEC OMP: 371.applu331/src/syncs.F90

@@ -82,6 +85,8 @@
 !$omp flush(isync)
          end do
+       CALL AnnotateHappensAfter(__FILE__, __LINE__, isync(omp_get_thread_num()))
+       CALL AnnotateHappensBefore(__FILE__, __LINE__, isync(iam))
          isync(iam) = 1
 !$omp flush(isync)
       endif
protze.joachim marked an inline comment as done.

Removed a file which was mistakenly added

More comments

tools/CMakeLists.txt
3

s/project/tool/

tools/archer/CMakeLists.txt
5

I think this should now say See https://llvm.org/LICENSE.txt for license information., at least that's what other files have.

18–19

Please use OPENMP_INSTALL_LIBDIR to handle libdir suffix.

tools/archer/ftsan.c
2 ↗(On Diff #200309)

It's not required for Archer and belongs into the sanitizer runtime.

tools/archer/ompt-tsan.cpp
204

This should be standard now.

545

Indention looks odd...

721–722

?

748

?

tools/archer/tests/CMakeLists.txt
6

Do they?

34–35

At least these two don't exist here.

tools/archer/tests/lit.cfg
106–109

Is this needed?

118–131

Is this needed?

tools/archer/tests/lit.site.cfg.in
14–16

This should not be needed

tools/archer/tests/races/critical-unrelated.c
38

This depends on the compiler used, right? Can the tests be executed by GCC?

protze.joachim marked 16 inline comments as done.Jul 17 2019, 9:41 AM
protze.joachim added inline comments.
tools/archer/ftsan.c
2 ↗(On Diff #200309)

It doesn't harm to have this duplicately. I'm not sure how this would fit into the LLVM/sanitizer as long as there is no Fortran frontend supporting this sanitizer.

I moved the code to the other declarations in ompt-tsan.cpp.

protze.joachim marked an inline comment as done.

Addressed comments by Jonas.
The tests also support gcc now.

Hahnfeld added inline comments.Jul 18 2019, 9:08 AM
tools/archer/ompt-tsan.cpp
176–193

Moving the code doesn't solve the underlying issues:

  1. It's not required for Archer, or at least for the basic tests to pass.
  2. It's not related to OpenMP per se.

Besides, with the current usage to dynamically load the library during runtime, it will not resolve symbols during linkage.

384–385

This comment doesn't sound right.

924–925

This functionality is not used in the tests and could be a separate patch.

Hahnfeld added inline comments.Jul 18 2019, 9:11 AM
tools/archer/tests/lit.site.cfg.in
14–15

Still not needed

30–52

Not needed

protze.joachim marked 6 inline comments as done.

Removed the Fortran annotation functions :/
Fixed also the other comments

tools/archer/ompt-tsan.cpp
924–925

This functionality is useful for debugging errors resulting from errors in the OMPT implementation.

Hahnfeld added inline comments.Jul 20 2019, 1:21 AM
tools/archer/ompt-tsan.cpp
924–925

I agree, and this sounds like a good motivation for a second tool. What's the advantage of having this in ompt-tsan.cpp?

Removed the counters as requested

The code itself looks good to me (it should probably be clang-formated). If others have comments, please raise them now.

Apart from that, I still found many unused lines in the testing framework...

tools/archer/README.md
168–177

Do we still want to speak of clang-archer?

tools/archer/tests/CMakeLists.txt
18–19

These variables are not defined, so I guess they can be safely removed

29–30

Likewise

tools/archer/tests/lit.cfg
53–62

I think these lines can be removed, see comments in CMakeLists.txt and implications

92–96

This can probably be removed

tools/archer/tests/lit.site.cfg.in
14–15

Ping, there's no definition of OPENMP_TEST_ARCHER_FLAGS and no use of test_archer_flags

18–24

All of these are either not used or are probably not needed, see comment in CMakeLists.txt.

tools/archer/tests/ompt/ompt-signal.h
2

We should probably work on deduplicating this from the OMPT tests. Not a strict requirement for now, the file is probably small enough...