This repository has been archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
Replace acquire+release thread annotation with excludes #5944
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The behavior of acquire+release annotation handling has changed in https://reviews.llvm.org/D49355 which breaks the build with the new Clang. However, as has been pointed out, the acquire+release isn't the right way to prevent double locking as the annotations negate each other; the correct way is to use excludes or negative requires. Using excludes annotations also requires using std::lock_guard instead of std::unique_lock because the latter doesn't have the thread annotations due to deferred locking which is not needed in Flutter and so std::lock_guard is a sufficient alternative.
bb9b5ad
to
1409e15
Compare
@chinmaygarde is a better reviewer than I am for this change. |
chinmaygarde
approved these changes
Aug 6, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Appreciate the links to the surrounding discussion.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 6, 2018
flutter/engine@4893b07...c6baaaf git log 4893b07..c6baaaf --date=short --no-merges --format='%%ad %%ae %%s' 2018-08-05 petrhosek@users.noreply.github.com Replace acquire+release thread annotation with excludes (flutter/engine#5944) 2018-08-05 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) 2018-08-03 liyuqian@google.com Call drawPaint instead of drawPath if there's clip (flutter/engine#5937) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia cdfa16d086b8..4c3b63e289c5 (9 commits) (flutter/engine#5936) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 1400d38e0863..cdfa16d086b8 (10 commits) (flutter/engine#5934) 2018-08-03 amirh@users.noreply.github.com Flush all embedded Android views on hot restart. (flutter/engine#5929) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 578ef2847b72..1400d38e0863 (7 commits) (flutter/engine#5933) 2018-08-02 amirh@users.noreply.github.com Don't drop MotionEvents with unknown tool type. (flutter/engine#5931) 2018-08-02 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 64cc576b1fa7..578ef2847b72 (20 commits) (flutter/engine#5930) 2018-08-02 regis@google.com Roll Dart to b04def964c428ada007cca7ef6b4936001db965d (flutter/engine#5928) 2018-08-02 jamesderlin@users.noreply.github.com Add an explicit `-[FlutterViewController init]` implementation (flutter/engine#5924) The AutoRoll server is located here: https://flutter-engine-flutter-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 6, 2018
flutter/engine@4893b07...f4464a8 git log 4893b07..f4464a8 --date=short --no-merges --format='%%ad %%ae %%s' 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) 2018-08-05 petrhosek@users.noreply.github.com Replace acquire+release thread annotation with excludes (flutter/engine#5944) 2018-08-05 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) 2018-08-03 liyuqian@google.com Call drawPaint instead of drawPath if there's clip (flutter/engine#5937) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia cdfa16d086b8..4c3b63e289c5 (9 commits) (flutter/engine#5936) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 1400d38e0863..cdfa16d086b8 (10 commits) (flutter/engine#5934) 2018-08-03 amirh@users.noreply.github.com Flush all embedded Android views on hot restart. (flutter/engine#5929) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 578ef2847b72..1400d38e0863 (7 commits) (flutter/engine#5933) 2018-08-02 amirh@users.noreply.github.com Don't drop MotionEvents with unknown tool type. (flutter/engine#5931) 2018-08-02 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 64cc576b1fa7..578ef2847b72 (20 commits) (flutter/engine#5930) 2018-08-02 regis@google.com Roll Dart to b04def964c428ada007cca7ef6b4936001db965d (flutter/engine#5928) 2018-08-02 jamesderlin@users.noreply.github.com Add an explicit `-[FlutterViewController init]` implementation (flutter/engine#5924) The AutoRoll server is located here: https://flutter-engine-flutter-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 6, 2018
flutter/engine@ecbb2b2...5770cbd git log ecbb2b2..5770cbd --date=short --no-merges --format='%%ad %%ae %%s' 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 12fb9cfeee07..6e487e67a3f3 (8 commits) (flutter/engine#5946) 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) 2018-08-05 petrhosek@users.noreply.github.com Replace acquire+release thread annotation with excludes (flutter/engine#5944) 2018-08-05 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) The AutoRoll server is located here: https://flutter-engine-flutter-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 6, 2018
flutter/engine@ecbb2b2...bc885f3 git log ecbb2b2..bc885f3 --date=short --no-merges --format='%%ad %%ae %%s' 2018-08-06 bkonyi@google.com Updated background execution implementation for Android 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 12fb9cfeee07..6e487e67a3f3 (8 commits) (flutter/engine#5946) 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) 2018-08-05 petrhosek@users.noreply.github.com Replace acquire+release thread annotation with excludes (flutter/engine#5944) 2018-08-05 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) The AutoRoll server is located here: https://flutter-engine-flutter-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 6, 2018
flutter/engine@ecbb2b2...a5215ce git log ecbb2b2..a5215ce --date=short --no-merges --format='%%ad %%ae %%s' 2018-08-06 jonahwilliams@google.com Add hasImplicitScrolling SemanticFlag and support in Android bridge (flutter/engine#5941) 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 6e487e67a3f3..641ac7daa81c (16 commits) (flutter/engine#5948) 2018-08-06 bkonyi@google.com Updated background execution implementation for Android 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 12fb9cfeee07..6e487e67a3f3 (8 commits) (flutter/engine#5946) 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) 2018-08-05 petrhosek@users.noreply.github.com Replace acquire+release thread annotation with excludes (flutter/engine#5944) 2018-08-05 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) The AutoRoll server is located here: https://flutter-engine-flutter-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 6, 2018
flutter/engine@ecbb2b2...5442c0a git log ecbb2b2..5442c0a --date=short --no-merges --format='%%ad %%ae %%s' 2018-08-06 alexmarkov@google.com Revert "Updated background execution implementation for Android" (flutter/engine#5949) 2018-08-06 jonahwilliams@google.com Add hasImplicitScrolling SemanticFlag and support in Android bridge (flutter/engine#5941) 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 6e487e67a3f3..641ac7daa81c (16 commits) (flutter/engine#5948) 2018-08-06 bkonyi@google.com Updated background execution implementation for Android 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 12fb9cfeee07..6e487e67a3f3 (8 commits) (flutter/engine#5946) 2018-08-06 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) 2018-08-05 petrhosek@users.noreply.github.com Replace acquire+release thread annotation with excludes (flutter/engine#5944) 2018-08-05 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) 2018-08-03 37626415+skia-flutter-autoroll@users.noreply.github.com Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) The AutoRoll server is located here: https://flutter-engine-flutter-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.
liyuqian
added a commit
to liyuqian/flutter
that referenced
this pull request
Aug 7, 2018
7f083e5 Don't implicitly fall through in switch statement (flutter/engine#5964) c7ce6dd Apply translation to accessibility tree when in landscape (flutter/engine#5950) aef94b7 Reland "Updated background execution implementation for Android" (flutter/engine#5954) 3421bca Roll src/third_party/skia 36216fb0acbc..177742435e52 (15 commits) (flutter/engine#5963) 9d1e673 Temporarily add travis/analyze.sh back for Chrome bot (flutter/engine#5961) 4386afd Roll src/third_party/skia e6d0618f677c..36216fb0acbc (11 commits) (flutter/engine#5960) 7cef3da Roll src/third_party/skia 01d9a344b575..e6d0618f677c (1 commits) (flutter/engine#5958) ad82cb1 Roll src/third_party/skia fdf05f4ff4e9..01d9a344b575 (1 commits) (flutter/engine#5957) 6ea410c Revert "Roll Dart to 17b54c76ce9b945c6f013ad08c19268409c0694a (flutter/engine#5955)" (flutter#5956) f20c58f Roll Dart to 17b54c76ce9b945c6f013ad08c19268409c0694a (flutter/engine#5955) ed1938e Roll src/third_party/skia 641ac7daa81c..fdf05f4ff4e9 (3 commits) (flutter/engine#5953) 5f04e00 Remove travis directory (flutter/engine#5935) 5442c0a Revert "Updated background execution implementation for Android" (flutter/engine#5949) a5215ce Add hasImplicitScrolling SemanticFlag and support in Android bridge (flutter/engine#5941) 4681351 Roll src/third_party/skia 6e487e67a3f3..641ac7daa81c (16 commits) (flutter/engine#5948) bc885f3 Updated background execution implementation for Android 5770cbd Roll src/third_party/skia 12fb9cfeee07..6e487e67a3f3 (8 commits) (flutter/engine#5946) f4464a8 Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) c6baaaf Replace acquire+release thread annotation with excludes (flutter/engine#5944) 63ede2e Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 97aea09 Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) aaf4a9a Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940)
liyuqian
added a commit
to flutter/flutter
that referenced
this pull request
Aug 7, 2018
* Roll engine to 7f083e5 7f083e5 Don't implicitly fall through in switch statement (flutter/engine#5964) c7ce6dd Apply translation to accessibility tree when in landscape (flutter/engine#5950) aef94b7 Reland "Updated background execution implementation for Android" (flutter/engine#5954) 3421bca Roll src/third_party/skia 36216fb0acbc..177742435e52 (15 commits) (flutter/engine#5963) 9d1e673 Temporarily add travis/analyze.sh back for Chrome bot (flutter/engine#5961) 4386afd Roll src/third_party/skia e6d0618f677c..36216fb0acbc (11 commits) (flutter/engine#5960) 7cef3da Roll src/third_party/skia 01d9a344b575..e6d0618f677c (1 commits) (flutter/engine#5958) ad82cb1 Roll src/third_party/skia fdf05f4ff4e9..01d9a344b575 (1 commits) (flutter/engine#5957) 6ea410c Revert "Roll Dart to 17b54c76ce9b945c6f013ad08c19268409c0694a (flutter/engine#5955)" (#5956) f20c58f Roll Dart to 17b54c76ce9b945c6f013ad08c19268409c0694a (flutter/engine#5955) ed1938e Roll src/third_party/skia 641ac7daa81c..fdf05f4ff4e9 (3 commits) (flutter/engine#5953) 5f04e00 Remove travis directory (flutter/engine#5935) 5442c0a Revert "Updated background execution implementation for Android" (flutter/engine#5949) a5215ce Add hasImplicitScrolling SemanticFlag and support in Android bridge (flutter/engine#5941) 4681351 Roll src/third_party/skia 6e487e67a3f3..641ac7daa81c (16 commits) (flutter/engine#5948) bc885f3 Updated background execution implementation for Android 5770cbd Roll src/third_party/skia 12fb9cfeee07..6e487e67a3f3 (8 commits) (flutter/engine#5946) f4464a8 Roll src/third_party/skia 2e77f54f46e8..12fb9cfeee07 (1 commits) (flutter/engine#5945) c6baaaf Replace acquire+release thread annotation with excludes (flutter/engine#5944) 63ede2e Roll src/third_party/skia 59aabbcf3b0d..2e77f54f46e8 (1 commits) (flutter/engine#5943) 97aea09 Roll src/third_party/skia 71fe8baccc01..59aabbcf3b0d (1 commits) (flutter/engine#5942) aaf4a9a Roll src/third_party/skia 4c3b63e289c5..71fe8baccc01 (3 commits) (flutter/engine#5940) * Sync framework changes
zanderso
pushed a commit
to zanderso/engine
that referenced
this pull request
Aug 10, 2018
The behavior of acquire+release annotation handling has changed in https://reviews.llvm.org/D49355 which breaks the build with the new Clang. However, as has been pointed out, the acquire+release isn't the right way to prevent double locking as the annotations negate each other; the correct way is to use excludes or negative requires. Using excludes annotations also requires using std::lock_guard instead of std::unique_lock because the latter doesn't have the thread annotations due to deferred locking which is not needed in Flutter and so std::lock_guard is a sufficient alternative.
zanderso
added a commit
that referenced
this pull request
Aug 10, 2018
The behavior of acquire+release annotation handling has changed in https://reviews.llvm.org/D49355 which breaks the build with the new Clang. However, as has been pointed out, the acquire+release isn't the right way to prevent double locking as the annotations negate each other; the correct way is to use excludes or negative requires. Using excludes annotations also requires using std::lock_guard instead of std::unique_lock because the latter doesn't have the thread annotations due to deferred locking which is not needed in Flutter and so std::lock_guard is a sufficient alternative.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The behavior of acquire+release annotation handling has changed in
https://reviews.llvm.org/D49355 which breaks the build with the new
Clang. However, as has been pointed out, the acquire+release isn't
the right way to prevent double locking as the annotations negate
each other; the correct way is to use excludes or negative requires.
Using excludes annotations also requires using std::lock_guard instead
of std::unique_lock because the latter doesn't have the thread
annotations due to deferred locking which is not needed in Flutter and
so std::lock_guard is a sufficient alternative.