Page MenuHomePhabricator

[OMPT] Add implementation and tests of Archer tool

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



The tool provides TSAN annotations for OpenMP synchronization. Enabled for
application execution by setting, 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
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

233–235 ↗(On Diff #200309)

These preprocessor guards are redundant.

240–245 ↗(On Diff #200309)

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

7 ↗(On Diff #200309)

Why do we need to handle this dummy?

1–4 ↗(On Diff #200309)

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

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.

2 ↗(On Diff #200309)


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

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)
protze.joachim marked an inline comment as done.

Removed a file which was mistakenly added

More comments

2 ↗(On Diff #210043)


4 ↗(On Diff #210043)

I think this should now say See for license information., at least that's what other files have.

17–18 ↗(On Diff #210043)

Please use OPENMP_INSTALL_LIBDIR to handle libdir suffix.

2 ↗(On Diff #200309)

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

203 ↗(On Diff #210043)

This should be standard now.

544 ↗(On Diff #210043)

Indention looks odd...

720–721 ↗(On Diff #210043)


747 ↗(On Diff #210043)


5 ↗(On Diff #210043)

Do they?

33–34 ↗(On Diff #210043)

At least these two don't exist here.

105–108 ↗(On Diff #210043)

Is this needed?

117–130 ↗(On Diff #210043)

Is this needed?

13–15 ↗(On Diff #210043)

This should not be needed

37 ↗(On Diff #210043)

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.
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
175–192 ↗(On Diff #210350)

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.

383–384 ↗(On Diff #210350)

This comment doesn't sound right.

923–924 ↗(On Diff #210350)

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

Hahnfeld added inline comments.Jul 18 2019, 9:11 AM
13–14 ↗(On Diff #210350)

Still not needed

29–51 ↗(On Diff #210350)

Not needed

protze.joachim marked 6 inline comments as done.

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

923–924 ↗(On Diff #210350)

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

Hahnfeld added inline comments.Jul 20 2019, 1:21 AM
923–924 ↗(On Diff #210350)

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...

168–177 ↗(On Diff #219468)

Do we still want to speak of clang-archer?

18–19 ↗(On Diff #219468)

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

29–30 ↗(On Diff #219468)


53–62 ↗(On Diff #219468)

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

92–96 ↗(On Diff #219468)

This can probably be removed

18–24 ↗(On Diff #219468)

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

13–14 ↗(On Diff #210350)

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

2 ↗(On Diff #219468)

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

This revision was not accepted when it landed; it landed in state Needs Review.Nov 18 2019, 5:46 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 5:46 AM
twoh added a subscriber: twoh.Nov 19 2019, 2:29 PM
twoh added inline comments.

@protze.joachim Shouldn't this be @LIBARCHER_HAVE_LIBATOMIC@ , based on openmp/tools/archer/tests/CMakeLists.txt? For our local unit test runs, they fail with

llvm-lit: /home/engshare/third-party2/llvm/src/llvm/utils/lit/lit/ fatal: unable to parse config file '/home/engshare/third-party2/llvm/src/build-centos7-native/build/projects/openmp/tools/archer/tests/', traceback: Traceback (most recent call last):
  File "/home/engshare/third-party2/llvm/src/llvm/utils/lit/lit/", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/home/engshare/third-party2/llvm/src/build-centos7-native/build/projects/openmp/tools/archer/tests/", line 14
    config.has_libatomic =
SyntaxError: invalid syntax

The newly added tests fail on buildbots:

Could you take a look? And please consider reverting in the meanwhile.

protze.joachim marked 2 inline comments as done.Nov 22 2019, 6:29 AM

The newly added tests fail on buildbots:

Could you take a look? And please consider reverting in the meanwhile.

Several things:

  • the tests rely on the availability of thread-sanitizer
  • I disabled the tests for standalone builds (as in the specific build-bot)
  • in a follow-up review, I'll enable the tests based on llvm-config information
  • MOST important: we should make the sanitizers available in the build-bot
  • MOST important: we should make the sanitizers available in the build-bot

May I ask you to make the necessary changes to the bot configuration? It is here:

  • the tests rely on the availability of thread-sanitizer

The tests should be annotated as REQUIRES: tsan or whatever the appropriate tag is.

  • the tests rely on the availability of thread-sanitizer

The tests should be annotated as REQUIRES: tsan or whatever the appropriate tag is.

I pushed this change, so that the tests are off currently.
I'll create a review to check and announce this capability. (@Hahnfeld I think, you have experience in testing the test-compiler for features?)

Thanks for the pointer to the buildbot configuration, I'll have a look!

Thank you very much for the quick workaround!

aaronpuchert added inline comments.

The commit message talks about setting, so why do we build and install this as static library?

If there are uses for the static library, could we decide based on LIBOMP_ENABLE_SHARED or BUILD_SHARED_LIBS whether to build it?