Valgrind and Thread Sanitizer (TSan) instrumentation

[vinser52]

During the investigation of the CI valgrind failures of PR #736, the concern raised in my mind if we correctly instrumented atomic-store-with-release and atomic-load-with-acquire operations.

Current instrumentation in UMF

In utils_concurrency.h there are two macros:

#define utils_atomic_load_acquire(object, dest)                                \
    do {                                                                       \
        utils_annotate_acquire((void *)object);                                \
        __atomic_load(object, dest, memory_order_acquire);                     \
    } while (0)

#define utils_atomic_store_release(object, desired)                            \
    do {                                                                       \
        __atomic_store_n(object, desired, memory_order_release);               \
        utils_annotate_release((void *)object);                                \
    } while (0)

The utils_annotate_acquire and utils_annotate_release functions defined in the utils_sanitizers.h as follows:

static inline void utils_annotate_acquire(void *ptr) {
#if __SANITIZE_THREAD__
    __tsan_acquire(ptr);
#elif UMF_VG_HELGRIND_ENABLED || UMF_VG_DRD_ENABLED
    ANNOTATE_HAPPENS_AFTER(ptr);
#else
    (void)ptr;
#endif
}

static inline void utils_annotate_release(void *ptr) {
#if __SANITIZE_THREAD__
    __tsan_release(ptr);
#elif UMF_VG_HELGRIND_ENABLED || UMF_VG_DRD_ENABLED
    ANNOTATE_HAPPENS_BEFORE(ptr);
#else
    (void)ptr;
#endif
}

My understanding of Annotation Placement:

Note: My understanding after searching in internet

1. For Valgrind (using ANNOTATE_HAPPENS_BEFORE and ANNOTATE_HAPPENS_AFTER):

• ANNOTATE_HAPPENS_BEFORE should be placed immediately before the actual atomic-store-with-release.
• ANNOTATE_HAPPENS_AFTER should be placed immediately after the actual atomic_load with acquire semantics.

3. For Thread Sanitizer (TSan) (using __tsan_release and __tsan_acquire):

• __tsan_release should be placed immediately after the actual atomic_store with release semantics.
• __tsan_acquire should be placed immediately after the actual atomic_load with acquire semantics.

[igchor]

It looks like the utils_annotate_* calls are actually placed incorrectly in those wrappers. I believe utils_annotate_acquire should be placed AFTER load and release BEFORE the store.

The behavior for TSAN's release/acquire seems to be the same as for Valgrind's BEFORE/AFTER, see here: https://github.com/llvm-mirror/compiler-rt/blob/69445f095c22aac2388f939bedebf224a6efcdaf/lib/tsan/tests/rtl/tsan_mutex.cpp#L189

We also have some places that just call utils_annotate_acquire/release manually, but they look OK (I might have missed something though).

[bratpiorka]

@vinser52, as @lplewa told me, the critnib in UMF uses empty Valgrind/DRD macros. It was originally copied from PMDK, and this was never completed. Will this PR pass with the implementation taken from PMDK?

VALGRIND_ANNOTATE_NEW_MEMORY: https://github.com/pmem/pmdk/blob/d18b20de96e3f663fbd364d9f96b8a61367d5afb/src/core/valgrind/drd.h#L402

VALGRIND_HG_DRD_DISABLE_CHECKING:
https://github.com/pmem/pmdk/blob/d18b20de96e3f663fbd364d9f96b8a61367d5afb/src/core/valgrind/helgrind.h#L393

[bratpiorka]

fixed by #1151