Fix TSAN warnings related to dfa::DFA::s0

[jcking]

In Java, dfa::DFA::s0 is marked as volatile and in Go it uses a mutex. The access is raw in C++ leading to TSAN warnings and undefined behavior if two threads update the pointer simultaneously. This is resolved by using std::atomic and acquire/release semantics.

[jcking]

@mike-lischke

[mike-lischke]

Hmm, what exactly is TSAN complaining about? You added atomics in a section which is already protected by a lock (see _stateLock.writeLock();). And that's not an average lock, but one which allows multiple reads at the same time to improve performance. What does your patch improve?

Could this be a false positive reported by TSAN?

[jcking]

Hmm, what exactly is TSAN complaining about? You added atomics in a section which is already protected by a lock (see _stateLock.writeLock();). And that's not an average lock, but one which allows multiple reads at the same time to improve performance. What does your patch improve?

Could this be a false positive reported by TSAN?

The race is in LexerATNSimulator::matchATN which I do not believe has mutation exclusion when updating the pointer.

[mike-lischke]

I'm still not convinced. Both lexer and parser ATN simulators have state and edge locks to prevent concurrent access. They protect not only the DFA s0 states but also the process of adding new DFA states. Adding std::atomic to the mix is not going to help here, but complicates things. So either std::atomic should go or the state/edge locks.

However I saw one case where no protection is in place: LexerATNSimulator::matchATN(). There's a write operation:

  if (!suppressEdge) {
    _decisionToDFA[_mode].s0 = next;
  }

which is not protected by the lock. Would be good to handle that properly by somehow extending the lock that was acquired in addDFAState to include this set operation. Maybe addDFAState can do the check for suppressing the edge and also set the s0 closure, which is currently done after addDFAState?

[jcking]

I'm still not convinced. Both lexer and parser ATN simulators have state and edge locks to prevent concurrent access. They protect not only the DFA s0 states but also the process of adding new DFA states. Adding std::atomic to the mix is not going to help here, but complicates things. So either std::atomic should go or the state/edge locks.

However I saw one case where no protection is in place: LexerATNSimulator::matchATN(). There's a write operation:

  if (!suppressEdge) {
    _decisionToDFA[_mode].s0 = next;
  }

which is not protected by the lock. Would be good to handle that properly by somehow extending the lock that was acquired in addDFAState to include this set operation. Maybe addDFAState can do the check for suppressing the edge and also set the s0 closure, which is currently done after addDFAState?

Here is the sanitized TSAN output.

WARNING: ThreadSanitizer: data race (pid=8618)
  Read of size 8 at 0x7b8400002838 by thread T73:
    #0 antlr4::atn::LexerATNSimulator::match(antlr4::CharStream*, unsigned long) atn/LexerATNSimulator.cpp:84:11
    #1 antlr4::Lexer::nextToken() Lexer.cpp:81:59
    #2 non-virtual thunk to antlr4::Lexer::nextToken() Lexer.cpp
    #3 antlr4::BufferedTokenStream::fetch(unsigned long) BufferedTokenStream.cpp:96:44
    #4 antlr4::BufferedTokenStream::sync(unsigned long) BufferedTokenStream.cpp:82:22
    #5 antlr4::BufferedTokenStream::setup() BufferedTokenStream.cpp:188:3
    #6 antlr4::BufferedTokenStream::lazyInit() BufferedTokenStream.cpp:182:5
    #7 antlr4::CommonTokenStream::LT(long) CommonTokenStream.cpp:44:3
    #8 antlr4::Parser::enterRule(antlr4::ParserRuleContext*, unsigned long, unsigned long) Parser.cpp:338:25

Note that a frame or two may be missing due to optimizations. We definitely need to have that write guarded by a mutex or std::atomic. Extending addDFAState to just do the work makes sense to me.

[jcking]

I think I have covered all the bases. I need to revisit the whole lock setup, as those _stateLock and _edgeLock should really not be static if possible. The state should be per LexerATNSimulator and ParserATNSimulator as they use separate data. So maybe the locks should be on the ATN.

[mike-lischke]

No, the locks must be static because they protect static data. The DFA is shared among all parser + lexer instances.

[mike-lischke]

@parrt Another pure C++ patch, ready for merge.