Page MenuHomePhabricator

[TSan] Do not compound reads and writes when separated by atomics
AbandonedPublic

Authored by melver on Jul 16 2020, 6:48 AM.

Details

Reviewers
dvyukov
glider
Summary

Do not compound reads and writes to the same address when separated by
atomic operations, including fences.

Diff Detail

Event Timeline

melver created this revision.Jul 16 2020, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2020, 6:48 AM

What would be an example of code where it matters?

What would be an example of code where it matters?

For the kernel it doesn't matter, because we don't use builtin atomics (yet). The asm case works because it looks like a call.

For normal code, you could have something like:

T0:
  x = load A  // data race
  atomic load acquire B // observes 1
  store A, ...

T1:
  store A, ...
  atomic store release B, 1

If the current logic compounds this and TSAN only sees:

T0:
  atomic load acquire B // observes 1
  store A, ...

we might miss the data race of the load on A. In practice, it probably doesn't really matter because T0 would probably load acquire spin on B or something better, and it'd be all in separate basic blocks.

In practice, it probably doesn't really matter because T0 would probably load acquire spin on B or something better, and it'd be all in separate basic blocks.

Yes, that's what I am thinking as well. This only makes difference for a ~1 instruction window for the context where everything is timing dependent anyway...

In practice, it probably doesn't really matter because T0 would probably load acquire spin on B or something better, and it'd be all in separate basic blocks.

Yes, that's what I am thinking as well. This only makes difference for a ~1 instruction window for the context where everything is timing dependent anyway...

Should we drop this? I think unless Alex has objections, I think it's fine to drop. I certainly don't feel too strongly either way.

I don't object against dropping it. I had another case in mind, which turned out to be an actual data race, not something that the compiler pulled out of thin air.

melver abandoned this revision.Jul 31 2020, 8:59 AM