Resolve data races found by ThreadSanitizer

[eszpotanski]

This PR fixes following data races found by ThreadSanitizer when OpenSTA is run by OpenROAD-flow-scripts:

• Accessing tags_ and tag_groups_ in sta::Search while they're being reallocated in another thread. It's not enough to delay the swap, as reading from an array is two operations on x86 (load the pointer, then load the array element). The array may get swapped out and freed in between those. Example warning:

  WARNING: ThreadSanitizer: data race (pid=2114735)
    Write of size 8 at 0x725c00000928 by thread T12 (mutexes: write M0):
      #0 sta::Search::findTagGroup(sta::TagGroupBldr*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2661 (openroad+0x10360e1)
      #1 sta::Search::setVertexArrivals(sta::Vertex*, sta::TagGroupBldr*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2683 (openroad+0x1040ee3)
      #2 sta::ArrivalVisitor::visit(sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:1172 (openroad+0x1044037)
      [...]
  
    Previous read of size 8 at 0x725c00000928 by thread T2:
      #0 sta::Search::tagGroup(sta::Vertex const*) const OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2790 (openroad+0x1046553)
      #1 sta::PathVisitor::visitEdge(sta::Pin const*, sta::Vertex*, sta::Edge*, sta::Pin const*, sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2041 (openroad+0x1046553)
      #2 sta::PathVisitor::visitFaninPaths(sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2006 (openroad+0x102bf95)
      [...]
  
  

• Accessing and modifying bit fields from multiple threads. It's not possible to write individual bits to RAM, the best you can do is read a byte, do a bit operation on it, and write it back. Moreover, compilers can (and do) read up to 8 bytes, do bit operations on that, and write the whole 8 bytes back. If between reading and writing a field we're not even accessing (seemingly) changes, then the write-back overwrites that change. Example warning:

  WARNING: ThreadSanitizer: data race (pid=2081413)
    Read of size 3 at 0x728804e234f0 by thread T2:
      #0 sta::Vertex::level() const OpenROAD-flow-scripts/tools/OpenROAD/src/sta/include/sta/Graph.hh:286 (openroad+0xfd2b06)
      #1 sta::BfsFwdIterator::enqueueAdjacentVertices(sta::Vertex*, sta::SearchPred*, int) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Bfs.cc:367 (openroad+0xfd2b06)
      #2 sta::BfsIterator::enqueueAdjacentVertices(sta::Vertex*, sta::SearchPred*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Bfs.cc:127 (openroad+0xfd0482)
      #3 sta::ArrivalVisitor::visit(sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:1167 (openroad+0x1043fe0)
      [...]
  
    Previous write of size 6 at 0x728804e234f0 by thread T15:
      #0 sta::Vertex::setTagGroupIndex(int) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/graph/Graph.cc:1263 (openroad+0xebcd28)
      #1 sta::Search::setVertexArrivals(sta::Vertex*, sta::TagGroupBldr*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2727 (openroad+0x104100a)
      #2 sta::ArrivalVisitor::visit(sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:1172 (openroad+0x1044037)
      [...]
  

• Indexing and inserting in an ObjectTable or ArrayTable in sta::Graph from multiple threads. Example warning:

  WARNING: ThreadSanitizer: data race (pid=2081413)
  Write of size 8 at 0x725800001310 by thread T14 (mutexes: write M0):
    #0 sta::ArrayTable<float>::pushBlock(sta::ArrayBlock<float>*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/include/sta/ArrayTable.hh:172 (openroad+0xec59e3)
    #1 sta::ArrayTable<float>::makeBlock(unsigned int) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/include/sta/ArrayTable.hh:150 (openroad+0xec145b)
    #2 sta::ArrayTable<float>::make(unsigned int, float*&, unsigned int&) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/include/sta/ArrayTable.hh:134 (openroad+0xec145b)
    [...]
  
  Previous read of size 8 at 0x725800001310 by thread T6:
    #0 sta::ArrayTable<float>::pointer(unsigned int) const OpenROAD-flow-scripts/tools/OpenROAD/src/sta/include/sta/ArrayTable.hh:201 (openroad+0xebaa51)
    #1 sta::Graph::arrivals(sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/graph/Graph.cc:589 (openroad+0xebaa51)
    #2 sta::PathVertex::arrival(sta::StaState const*) const OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/PathVertex.cc:232 (openroad+0x1002349)
    #3 sta::PathVisitor::visitFromPath(sta::Pin const*, sta::Vertex*, sta::RiseFall const*, sta::PathVertex*, sta::Edge*, sta::TimingArc*, sta::Pin const*, sta::Vertex*, sta::RiseFall const*, sta::MinMax const*, sta::PathAnalysisPt const*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/Search.cc:2106 (openroad+0x10391cb)
    [...]
  

• A race between PathGroup::savable which accesses PathEnds and PathGroup::prune() which deletes PathEnds. Example warning:

  WARNING: ThreadSanitizer: data race (pid=2898495)
    Write of size 4 at 0x72200032a4cc by thread T13 (mutexes: write M0):
      #0 sta::PathGroup::prune() OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/PathGroup.cc:155 (openroad+0xffa7b3)
      #1 sta::PathGroup::insert(sta::PathEnd*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/PathGroup.cc:128 (openroad+0xffaa97)
      #2 sta::MakePathEnds1::vertexEnd(sta::Vertex*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/PathGroup.cc:578 (openroad+0xffab62)
      [...]
  
    Previous read of size 4 at 0x72200032a4cc by thread T14:
      #0 sta::delayLessEqual(float const&, float const&, sta::StaState const*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/graph/DelayFloat.cc:120 (openroad+0xeb5dad)
      #1 sta::PathGroup::savable(sta::PathEnd*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/PathGroup.cc:104 (openroad+0xff19ea)
      #2 sta::MakePathEnds1::visitPathEnd(sta::PathEnd*, sta::PathGroup*) OpenROAD-flow-scripts/tools/OpenROAD/src/sta/search/PathGroup.cc:553 (openroad+0xff8764)
      [...]
  

These changes incur a significant slowdown (between 10% and 40%, depending on number of threads, used design or stage).

Note that these are only the races found by ThreadSanitizer, not necessarily all of them.

[CLAassistant]

All committers have signed the CLA.

[QuantamHD]

How does this compare to just locking. I wonder if the additional memory locality and smaller memory footprint make up for the coarser lock

[eszpotanski]

Locking is slightly slower - a few benchmarks show slowdown between 0.5% and 2%

[QuantamHD]

@jjcherry56 Do these changes seem reasonable, or is there a better way to address these data races? The atomic bitset seems a bit complicated to me, but it might be worth the speed up.

[eszpotanski]

As b4be3c5 fixed part of the data races, I've updated this PR to contain fixes for remaining ones - caused by:

• accessing and modifying bit fields,
• indexing and inserting in an ObjectTable or ArrayTable.

[QuantamHD]

This should be resolved by 8797ac5 and 0dfea7d

[kbieganski]

TSan still complains on current OpenSTA master. Tested on ORFS/asap7/ibex:

• Usual TSan log for floorplan, CTS
• TSan log in the (rarer) case where OpenROAD crashes in floorplan

[QuantamHD]

It looks like in the latest version bfs_in_queue_ has been moved to std::atomic it looks safe to me. Is that your understanding as well @kbieganski

[kbieganski]

The bfs_in_queue_ seems safe, you can disregard the part of this PR that changes its accessors.

The remaining TSan warnings relate to the bitfields – you can't read or write a single bit, so reading the object index on one thread and setting a flag on another thread is seen as a data race as they're under the same address. I guess the object index doesn't change? So it could be safe.

But last time we looked at it, we still observed some crashes. We can take a look again.