Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

tests: enable helgrind and drd checkers for 'threads' tests #101

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lukaszstolarczuk
Copy link
Member

@lukaszstolarczuk lukaszstolarczuk commented Jul 7, 2022

I believe there might be some issue in threads mover, reported by helgrind/drd... or just a missing suppress.


This change is Reviewable

@lukaszstolarczuk lukaszstolarczuk added the help wanted Extra attention is needed label Jul 7, 2022
@igchor
Copy link

igchor commented Jul 7, 2022

I think this load should be atomic: https://github.com/pmem/miniasync/blob/master/src/core/ringbuf.c#L228

and there should be some happens before/after annotation for storing/loading completion flag: https://github.com/pmem/miniasync/blob/master/src/data_mover_threads.c#L168

This is what thread sanitizer reported.

@lukaszstolarczuk
Copy link
Member Author

huh, ok, so your proposed fixed is required... (or at least a part of it)?
// pmem/pmemstream#235 (comment)

@lukaszstolarczuk
Copy link
Member Author

we may also need to add suppressions/annotations for thread-sanitizer.

See TSAN support in pmemstream: https://github.com/pmem/pmemstream/pull/234/files
See our annotations: https://github.com/pmem/pmemstream/pull/236/files#diff-04996d3777968a283c7157ae594beacf8a4642d699a2c6e21a558213e867ab37

@pbalcer
Copy link
Member

pbalcer commented Jul 11, 2022

I fixed the ringbuf stuff in #103. Can you show me an example of how to do the annotations?

@igchor
Copy link

igchor commented Jul 11, 2022

@pbalcer I think you only need to add __tsan_acquire/release (https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/sanitizer/tsan_interface.h#L24) where you would normally put memory fences. I also belive that you could just add __tsan_acquire as part of VALGRIND_ANNOTATE_HAPPENS_AFTER (and similarly for release).

E.g. for data_mover_threads.c you would need to add __tsan_release before

util_atomic_store_explicit64(&data->complete, 1, memory_order_release);
and __tsan_acquire after
util_atomic_load_explicit64(&tdata->complete,

For ringbuf.c, I'm actually not sure how many extra annotations are needed (I did not run any miniasync tests with tsan). In general, the safest approach would probably be to annotate every atomic store with memory order different than relaxed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants