This is an archive of the discontinued LLVM Phabricator instance.

tsan: add new vector clock
ClosedPublic

Authored by dvyukov on Jul 30 2021, 7:48 AM.

Details

Summary

Add new fixed-size vector clock for the new tsan runtime.
For now it's unused.

Diff Detail

Event Timeline

dvyukov created this revision.Jul 30 2021, 7:48 AM
dvyukov requested review of this revision.Jul 30 2021, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2021, 7:48 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
dvyukov updated this revision to Diff 363096.Jul 30 2021, 8:01 AM

added some comments

Review incomplete -- I'll need to look a bit at the rest of the (uncommitted) changes to understand the requirements. But if there are special requirements that aren't obvious, do let us know.

Trying to review the SSE code as well.

compiler-rt/lib/tsan/rtl/tsan_defs.h
59

Any overloaded operators / helpers for these that should be pulled into this patch?

compiler-rt/lib/tsan/rtl/tsan_vector_clock.h
35

Is this the only required operator?
No operator==, operator<, or such required?

But if there are special requirements that aren't obvious, do let us know.

Humm... I can't think of any.

dvyukov added inline comments.Jul 30 2021, 8:45 AM
compiler-rt/lib/tsan/rtl/tsan_defs.h
59

I don't have any in the larger PR.

compiler-rt/lib/tsan/rtl/tsan_vector_clock.h
35

Is this the only required operator?

Yes.

No operator==, operator<, or such required?

No.

melver accepted this revision.Aug 2 2021, 2:49 AM
melver added inline comments.
compiler-rt/lib/tsan/rtl/tsan_defs.h
56

I infer that this is not actually max Sid (!= 255), because if this were to be stored in Sid it'd overflow. (Since kMaxSid and Sid are incompatible types my guess is that's not happening right now.)

kMaxThreadSlots ?

compiler-rt/lib/tsan/rtl/tsan_vector_clock.cpp
69

Would be good to make clang-tidy happy unless there's a performance benefit to not have it return *this.

This revision is now accepted and ready to land.Aug 2 2021, 2:49 AM
dvyukov added inline comments.Aug 2 2021, 4:34 AM
compiler-rt/lib/tsan/rtl/tsan_defs.h
56

Right, it's not "max", it's "count".
kThreadSlotCount?

melver added inline comments.Aug 2 2021, 4:37 AM
compiler-rt/lib/tsan/rtl/tsan_defs.h
56

Sounds good. That name to me suggests that there are always kThreadSlotCount slots (no more no less), right?

dvyukov marked an inline comment as done.Aug 2 2021, 4:42 AM
dvyukov added inline comments.
compiler-rt/lib/tsan/rtl/tsan_defs.h
56

Yes, my current prototype uses fixed number of slots always.

dvyukov updated this revision to Diff 363443.Aug 2 2021, 4:44 AM

rename kMaxSid to kThreadSlotCount
fixed operator= signature

This revision was landed with ongoing or failed builds.Aug 2 2021, 4:45 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka added inline comments.Nov 14 2021, 6:22 PM
compiler-rt/lib/tsan/rtl/tsan_defs.h
55

Why this is "enum class"?

dvyukov added inline comments.Nov 14 2021, 11:44 PM
compiler-rt/lib/tsan/rtl/tsan_defs.h
55

Is you mean "instead of typedef", enum class won't convert to any other types implicitly and gives much stronger type safety.