Page MenuHomePhabricator

[OpenMP][Tool] Runtime warning for missing TSan-option
Needs ReviewPublic

Authored by protze.joachim on Nov 18 2019, 3:36 PM.

Details

Summary

TSan reports for any OpenMP application a race on the initialization of a runtime internal mutex:

Atomic read of size 1 at 0x7b6800005940 by thread T4:
  #0 pthread_mutex_lock <null> (a.out+0x43f39e)
  #1 __kmp_resume_64 <null> (libomp.so.5+0x84db4)

Previous write of size 1 at 0x7b6800005940 by thread T7:
  #0 pthread_mutex_init <null> (a.out+0x424793)
  #1 __kmp_suspend_initialize_thread <null> (libomp.so.5+0x8422e)

According to @AndreyChurbanov this is a false positiv report, as the control flow of the runtime guarantees the ordering of the mutex initialization and the lock: https://software.intel.com/en-us/forums/intel-open-source-openmp-runtime-library/topic/530363

To suppress this report, I suggest the use of TSAN_OPTIONS='ignore_uninstrumented_modules=1'. With this patch, a runtime warning is provided in case an OpenMP application is built with Tsan and executed without this Tsan-option.

Diff Detail

Event Timeline

protze.joachim created this revision.Nov 18 2019, 3:36 PM

Attached the right diff.

Wasn't the conclusion of the intel forum thread that we should remove a call? Is this still necessary afterward?

The conclusion of the discussion in the intel forum thread was, that the call from __kmp_resume to __kmp_suspend_initialize_thread is superflous, because the thread will call __kmp_suspend_initialize_thread always during the own initialization before changing a variable to signal readiness.

The conclusion of the discussion in the intel forum thread was, that the call from __kmp_resume to __kmp_suspend_initialize_thread is superflous, because the thread will call __kmp_suspend_initialize_thread always during the own initialization before changing a variable to signal readiness.

But that doesn't make this obsolete?

This looks generally fine to me I just want to understand why we can't fix the runtime instead.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jan 14, 12:02 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 14, 12:02 PM
Hahnfeld reopened this revision.Tue, Jan 14, 12:21 PM

This revision was not accepted before being committed!

openmp/tools/archer/ompt-tsan.cpp
868

This memory is leaked, it should be deleted in ompt_tsan_finalize

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

Now that the patch is already in the repository, I don't know how much sense it would make to revert it in full. We should still fix the memory leak asap and make sure it's in the release.

openmp/tools/archer/ompt-tsan.cpp
868

Do we really need global lifetime for tsan_flags? Right now it would be enough to just have the object allocated on the stack and let the compiler handle the cleanup.

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

I did say I was generally fine with this but it should not trigger if we build the runtime with tsan instrumentation.
That, and especially the memory leak @Hahnfeld found, need to be taken care of ASAP.

Also, when was the CREDITS.txt change added?