Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contributors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -318,4 +318,4 @@ YYYY/MM/DD, github id, Full name, email
2021/09/23, skalt, Steven Kalt, [email protected]
2021/10/10, tools4origins, Erwan Guyomarc'h, [email protected]
2021/10/19, jcking, Justin King, [email protected]
2021/10/31, skef, Skef Iterum, [email protected]
2021/10/31, skef, Skef Iterum, [email protected]
22 changes: 16 additions & 6 deletions runtime/Cpp/runtime/src/atn/LexerATNSimulator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,13 @@ size_t LexerATNSimulator::match(CharStream *input, size_t mode) {
_startIndex = input->index();
_prevAccept.reset();
const dfa::DFA &dfa = _decisionToDFA[mode];
if (dfa.s0 == nullptr) {
_stateLock.readLock();
dfa::DFAState* s0 = dfa.s0;
_stateLock.readUnlock();
if (s0 == nullptr) {
return matchATN(input);
} else {
return execATN(input, dfa.s0);
return execATN(input, s0);
}
}

Expand Down Expand Up @@ -111,10 +114,7 @@ size_t LexerATNSimulator::matchATN(CharStream *input) {
bool suppressEdge = s0_closure->hasSemanticContext;
s0_closure->hasSemanticContext = false;

dfa::DFAState *next = addDFAState(s0_closure.release());
if (!suppressEdge) {
_decisionToDFA[_mode].s0 = next;
}
dfa::DFAState *next = addDFAState(s0_closure.release(), suppressEdge);

size_t predict = execATN(input, next);

Expand Down Expand Up @@ -536,6 +536,10 @@ void LexerATNSimulator::addDFAEdge(dfa::DFAState *p, size_t t, dfa::DFAState *q)
}

dfa::DFAState *LexerATNSimulator::addDFAState(ATNConfigSet *configs) {
return addDFAState(configs, true);
}

dfa::DFAState *LexerATNSimulator::addDFAState(ATNConfigSet *configs, bool suppressEdge) {
/* the lexer evaluates predicates on-the-fly; by this point configs
* should not contain any configurations with unevaluated predicates.
*/
Expand Down Expand Up @@ -563,6 +567,9 @@ dfa::DFAState *LexerATNSimulator::addDFAState(ATNConfigSet *configs) {
auto iterator = dfa.states.find(proposed);
if (iterator != dfa.states.end()) {
delete proposed;
if (!suppressEdge) {
_decisionToDFA[_mode].s0 = *iterator;
}
_stateLock.writeUnlock();
return *iterator;
}
Expand All @@ -572,6 +579,9 @@ dfa::DFAState *LexerATNSimulator::addDFAState(ATNConfigSet *configs) {
proposed->configs->setReadonly(true);

dfa.states.insert(proposed);
if (!suppressEdge) {
_decisionToDFA[_mode].s0 = proposed;
}
_stateLock.writeUnlock();

return proposed;
Expand Down
2 changes: 2 additions & 0 deletions runtime/Cpp/runtime/src/atn/LexerATNSimulator.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ namespace atn {
/// </summary>
virtual dfa::DFAState *addDFAState(ATNConfigSet *configs);

virtual dfa::DFAState *addDFAState(ATNConfigSet *configs, bool suppressEdge);

public:
dfa::DFA& getDFA(size_t mode);

Expand Down
14 changes: 10 additions & 4 deletions runtime/Cpp/runtime/src/atn/ParserATNSimulator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,30 +94,35 @@ size_t ParserATNSimulator::adaptivePredict(TokenStream *input, size_t decision,
});

dfa::DFAState *s0;
_stateLock.readLock();
if (dfa.isPrecedenceDfa()) {
// the start state for a precedence DFA depends on the current
// parser precedence, and is provided by a DFA method.
_edgeLock.readLock();
s0 = dfa.getPrecedenceStartState(parser->getPrecedence());
_edgeLock.readUnlock();
} else {
// the start state for a "regular" DFA is just s0
s0 = dfa.s0;
}
_stateLock.readUnlock();

if (s0 == nullptr) {
bool fullCtx = false;
std::unique_ptr<ATNConfigSet> s0_closure = computeStartState(dynamic_cast<ATNState *>(dfa.atnStartState),
&ParserRuleContext::EMPTY, fullCtx);

_stateLock.writeLock();
dfa::DFAState* ds0 = dfa.s0;
if (dfa.isPrecedenceDfa()) {
/* If this is a precedence DFA, we use applyPrecedenceFilter
* to convert the computed start state to a precedence start
* state. We then use DFA.setPrecedenceStartState to set the
* appropriate start state for the precedence level rather
* than simply setting DFA.s0.
*/
dfa.s0->configs = std::move(s0_closure); // not used for prediction but useful to know start configs anyway
dfa::DFAState *newState = new dfa::DFAState(applyPrecedenceFilter(dfa.s0->configs.get())); /* mem-check: managed by the DFA or deleted below */
ds0->configs = std::move(s0_closure); // not used for prediction but useful to know start configs anyway
dfa::DFAState *newState = new dfa::DFAState(applyPrecedenceFilter(ds0->configs.get())); /* mem-check: managed by the DFA or deleted below */
s0 = addDFAState(dfa, newState);
dfa.setPrecedenceStartState(parser->getPrecedence(), s0, _edgeLock);
if (s0 != newState) {
Expand All @@ -127,8 +132,9 @@ size_t ParserATNSimulator::adaptivePredict(TokenStream *input, size_t decision,
dfa::DFAState *newState = new dfa::DFAState(std::move(s0_closure)); /* mem-check: managed by the DFA or deleted below */
s0 = addDFAState(dfa, newState);

if (dfa.s0 != s0) {
delete dfa.s0; // Delete existing s0 DFA state, if there's any.
if (ds0 != s0) {
delete ds0; // Delete existing s0 DFA state, if there's any.
ds0 = nullptr;
dfa.s0 = s0;
}
if (s0 != newState) {
Expand Down
6 changes: 3 additions & 3 deletions runtime/Cpp/runtime/src/dfa/DFA.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,12 @@ DFA::DFA(atn::DecisionState *atnStartState, size_t decision)
}
}

DFA::DFA(DFA &&other) : atnStartState(other.atnStartState), decision(other.decision) {
DFA::DFA(DFA &&other) : atnStartState(other.atnStartState), s0(other.s0), decision(other.decision) {
// Source states are implicitly cleared by the move.
states = std::move(other.states);

other.atnStartState = nullptr;
other.decision = 0;
s0 = other.s0;
other.s0 = nullptr;
_precedenceDfa = other._precedenceDfa;
other._precedenceDfa = false;
Expand All @@ -52,8 +51,9 @@ DFA::~DFA() {
delete state;
}

if (!s0InList)
if (!s0InList) {
delete s0;
}
}

bool DFA::isPrecedenceDfa() const {
Expand Down