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

[silgenpattern] Fix two ASAN errors. #23801

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Apr 4, 2019

rdar://problem/49609717

gottesmm added 2 commits April 4, 2019 13:34
…one statement.

The problem would occur if after we accessed the value, we grew the DenseMap to
insert the value again. But putting in an extra auto I avoid the problem here.

rdar://49609717
…ing over an Optional<ArrayRef<T>>.

Specifically the bad pattern was:

```
   for (auto *vd : *caseStmt->getCaseBodyVariables()) { ... }
```

The problem is that the optional is not lifetime extended over the for loop. To
work around this, I changed the API of CaseStmt's getCaseBodyVariable methods to
never return the inner Optional<MutableArrayRef<T>>. Now we have the following 3
methods (ignoring const differences):

1. CaseStmt::hasCaseBodyVariables().

2. CaseStmt::getCaseBodyVariables(). Asserts if the case body variable array was
   never specified.

3. CaseStmt::getCaseBodyVariablesOrEmptyArray(). Returns either the case body
   variables array or an empty array if we were never given any case body
   variable array.

This should prevent anyone else in the future from hitting this type of bug.

radar://49609717
@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci asan test

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

NOTE: The second commit doesn't have any added tests since the tests we already have were asserting with asan enabled.

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

gottesmm commented Apr 4, 2019

I tested this with asan locally. Everything passed. Merging!

@gottesmm gottesmm merged commit b6c3ca0 into swiftlang:master Apr 4, 2019
@gottesmm gottesmm deleted the pr-56c61430037696146352efbfcedf418a94036015 branch April 4, 2019 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant