Add multithread support to ASan #9076
Merged
Conversation
This file contains hidden or 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
tlively
approved these changes
Aug 2, 2019
| @@ -3911,6 +3911,19 @@ def test_pthread_tls_main(self): | |||
| def test_pthread_lsan(self, name, args=[]): | |||
| self.btest(path_from_root('tests', 'pthread', name + '.cpp'), expected='1', args=['-fsanitize=leak', '-s', 'TOTAL_MEMORY=256MB', '-s', 'USE_PTHREADS', '-s', 'PROXY_TO_PTHREAD', '-std=c++11', '--pre-js', path_from_root('tests', 'pthread', name + '.js')] + args) | |||
|
|
|||
| @parameterized({ | |||
| 'leak': ['test_pthread_lsan_leak', ['-g4']], | |||
| 'no_leak': ['test_pthread_lsan_no_leak'], | |||
There was a problem hiding this comment.
No. We are reusing the test_pthread_lsan* for ASan. I'll add a comment to say this is not an error.
kripken
reviewed
Aug 2, 2019
| EMSCRIPTEN_KEEPALIVE | ||
| __attribute__((constructor(99))) // This must run before any userland ctors | ||
| __attribute__((constructor(0))) |
There was a problem hiding this comment.
Before this we had 99 and 100, so there was an order, but this changes them both to 0 - isn't that wrong? I think we need to change asan's priority.
There was a problem hiding this comment.
The two constructors don't actually depend on each other. We only thought they did due to pthread_cleanup, but as it turns out, pthread_cleanup is purely JavsScript and does not care if the C side has been initialized.
There was a problem hiding this comment.
Fair point. However, I think it would be safer going forward to have a definite order there - I can see us adding more stuff to one of the constructors some day, and having flaky failures because the order is nondeterministic. So I think it's worth changing asan's priority, keeping all three in a clear order.
There was a problem hiding this comment.
Diff to change ASan's priority: https://reviews.llvm.org/D65684
llvm-git-migration
pushed a commit
to llvm/llvm-project
that referenced
this pull request
Aug 6, 2019
Summary: This change gives Emscripten the ability to use more than one constructor priorities that runs before ASan. By convention, constructor priorites 0-100 are reserved for use by the system. ASan on Emscripten now uses priority 50, leaving plenty of room for use by Emscripten before and after ASan. This change is done in response to: emscripten-core/emscripten#9076 (comment) Reviewers: kripken, tlively, aheejin Reviewed By: tlively Subscribers: cfe-commits, dschuff, sbc100, jgravelle-google, hiraditya, sunfish, llvm-commits Tags: #llvm, #clang Differential Revision: https://reviews.llvm.org/D65684 llvm-svn: 368101
dtzWill
pushed a commit
to llvm-mirror/clang
that referenced
this pull request
Aug 6, 2019
Summary: This change gives Emscripten the ability to use more than one constructor priorities that runs before ASan. By convention, constructor priorites 0-100 are reserved for use by the system. ASan on Emscripten now uses priority 50, leaving plenty of room for use by Emscripten before and after ASan. This change is done in response to: emscripten-core/emscripten#9076 (comment) Reviewers: kripken, tlively, aheejin Reviewed By: tlively Subscribers: cfe-commits, dschuff, sbc100, jgravelle-google, hiraditya, sunfish, llvm-commits Tags: #llvm, #clang Differential Revision: https://reviews.llvm.org/D65684 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@368101 91177308-0d34-0410-b5e6-96231b3b80d8
dtzWill
pushed a commit
to llvm-mirror/llvm
that referenced
this pull request
Aug 6, 2019
Summary: This change gives Emscripten the ability to use more than one constructor priorities that runs before ASan. By convention, constructor priorites 0-100 are reserved for use by the system. ASan on Emscripten now uses priority 50, leaving plenty of room for use by Emscripten before and after ASan. This change is done in response to: emscripten-core/emscripten#9076 (comment) Reviewers: kripken, tlively, aheejin Reviewed By: tlively Subscribers: cfe-commits, dschuff, sbc100, jgravelle-google, hiraditya, sunfish, llvm-commits Tags: #llvm, #clang Differential Revision: https://reviews.llvm.org/D65684 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@368101 91177308-0d34-0410-b5e6-96231b3b80d8
spurious
pushed a commit
to spurious/clang-mirror
that referenced
this pull request
Aug 6, 2019
Summary: This change gives Emscripten the ability to use more than one constructor priorities that runs before ASan. By convention, constructor priorites 0-100 are reserved for use by the system. ASan on Emscripten now uses priority 50, leaving plenty of room for use by Emscripten before and after ASan. This change is done in response to: emscripten-core/emscripten#9076 (comment) Reviewers: kripken, tlively, aheejin Reviewed By: tlively Subscribers: cfe-commits, dschuff, sbc100, jgravelle-google, hiraditya, sunfish, llvm-commits Tags: #llvm, #clang Differential Revision: https://reviews.llvm.org/D65684 git-svn-id: http://llvm.org/svn/llvm-project/cfe/trunk@368101 91177308-0d34-0410-b5e6-96231b3b80d8
kripken
approved these changes
Aug 7, 2019
earl
pushed a commit
to earl/llvm-mirror
that referenced
this pull request
Aug 17, 2019
Summary: This change gives Emscripten the ability to use more than one constructor priorities that runs before ASan. By convention, constructor priorites 0-100 are reserved for use by the system. ASan on Emscripten now uses priority 50, leaving plenty of room for use by Emscripten before and after ASan. This change is done in response to: emscripten-core/emscripten#9076 (comment) Reviewers: kripken, tlively, aheejin Reviewed By: tlively Subscribers: cfe-commits, dschuff, sbc100, jgravelle-google, hiraditya, sunfish, llvm-commits Tags: #llvm, #clang Differential Revision: https://reviews.llvm.org/D65684 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@368101 91177308-0d34-0410-b5e6-96231b3b80d8
belraquib
pushed a commit
to belraquib/emscripten
that referenced
this pull request
Dec 23, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on #9073.