Skip to content

[TSan] Add positive test for TSan + Dispatch on Linux #24756

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 11, 2019
Merged

[TSan] Add positive test for TSan + Dispatch on Linux #24756

merged 2 commits into from
Jun 11, 2019

Conversation

yln
Copy link
Contributor

@yln yln commented May 14, 2019 •
edited
Loading

  1. Enable tests that use import Dispatch on Linux. Add substitution
    %import-libdispatch that needs to be used for all cross-platform
    tests (i.e., tests that are intended to be run on other platforms
    than Darwin) that do import Dispatch or enable thread sanitizer.

  2. Make sure as many existing Dispatch and TSan tests as possible run on
    Linux. Mark tests that would require substantial work with
    UNSUPPORTED: OS=linux-gnu.

  3. Add integration-style Swift test that shows that TSan finds a simple
    race when using Dispatch.async incorrectly. A more complete test
    suite for TSan's libdispatch support lives on the LLVM/compiler-rt
    side.

rdar://problem/49177535

@yln yln self-assigned this May 14, 2019
yln
yln commented May 14, 2019
View reviewed changes
@yln yln force-pushed the tsan-positive-tests branch from 898ce27 to c5dab03 Compare May 17, 2019 01:01
compnerd
compnerd previously requested changes May 17, 2019
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is most certainly going to break windows. There is a community CI up now, you should be able to do a one-off run on there. Can you add notes to the tests that you marked as unsupported on Linux as to why they are unsupported?

@swiftlang swiftlang deleted a comment from swift-ci May 17, 2019
@yln
Copy link
Contributor Author

yln commented May 17, 2019

@swift-ci Please test

@swiftlang swiftlang deleted a comment from swift-ci May 17, 2019
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - d2e8381

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - d2e8381

@yln
Copy link
Contributor Author

yln commented May 18, 2019

This is most certainly going to break windows. There is a community CI up now, you should be able to do a one-off run on there. Can you add notes to the tests that you marked as unsupported on Linux as to why they are unsupported?

Ah, I missed that before. Can you give/point me to the instructions for doing this?

@yln yln force-pushed the tsan-positive-tests branch from d2e8381 to d4619ae Compare May 20, 2019 17:25
@yln
Copy link
Contributor Author

yln commented May 20, 2019

@swift-ci Please test

@yln yln requested a review from compnerd May 20, 2019 20:26
@yln
Copy link
Contributor Author

yln commented May 20, 2019

@gottesmm @jrose-apple
The previous CI run succeeded. Assuming the current one does too, is this good to go?

@davezarzycki
Copy link
Contributor

Since @compnerd mentioned me in this pull request, I just did a clean build of this change using a unified (not build-script) setup (Red Hat Fedora 30), and ninja check-swift-validation passes. That being said, I'm not trying to test TSan.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - f019ba3

@yln yln force-pushed the tsan-positive-tests branch from f019ba3 to 2532380 Compare May 20, 2019 23:13
@yln
Copy link
Contributor Author

yln commented May 20, 2019

@swift-ci Please test

@compnerd
Copy link
Member

I just did a run of this on Windows and only the following test fails:

Swift(windows-x86_64) :: IRGen/tsan_coroutines.swift

@yln yln force-pushed the tsan-positive-tests branch from 2532380 to e2589e9 Compare May 22, 2019 18:37
@yln
Copy link
Contributor Author

yln commented May 22, 2019

@swift-ci Please test

jrose-apple
jrose-apple reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to delay on this. This doesn't have to block merging, but I don't love %import-libdispatch. What do you all think about adding these search paths unconditionally on Linux?

@yln
Copy link
Contributor Author

yln commented May 28, 2019

... I don't love %import-libdispatch.

I feel the same. The reason for it being like this is that it affects a number of substitutions, e.g., %target-swift-emit-*, %target-swift-frontend, %target-run, %target-swiftc_driver and the current approach meant a smaller, more manageable change.

@yln
Copy link
Contributor Author

yln commented May 28, 2019

@swift-ci Please test

@yln yln force-pushed the tsan-positive-tests branch from bea09e8 to 02688df Compare May 29, 2019 18:49
@yln
Copy link
Contributor Author

yln commented May 29, 2019

Force push to squash commit with uninteresting changes:

  1. Align indentation with surrounding code/use tabs.
  2. Rename variables.

@swift-ci Please test

@yln
Copy link
Contributor Author

yln commented May 29, 2019

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 02688df

@yln
Copy link
Contributor Author

yln commented May 30, 2019

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b8f5715

@yln
Copy link
Contributor Author

yln commented May 30, 2019

@compnerd @gottesmm @jrose-apple
I hope I have sufficiently addressed your feedback.

The Swift test suite passes. One lldb test timed out. Can we land this?

Timed Out Tests (1):
    lldb-Suite :: lang/swift/stepping/TestSwiftStepping.py

@yln
Copy link
Contributor Author

yln commented May 31, 2019

@swift-ci Please test OS X platform

@yln yln requested a review from compnerd May 31, 2019 20:09
yln added 2 commits June 10, 2019 14:24
@yln
[TSan] Add positive test for TSan + Dispatch on Linux
105e4ad 
1) Enable tests that use `import Dispatch` on Linux. Add substitution
   `%import-libdispatch` that needs to be used for all cross-platform
   tests (i.e., tests that are intended to be run on other platforms
   than Darwin) that do `import Dispatch` or enable thread sanitizer.

2) Make sure as many existing Dispatch and TSan tests as possible run on
   Linux. Mark tests that would require substantial work with
   `UNSUPPORTED: OS=linux-gnu`.

3) Add integration-style Swift test that shows that TSan finds a simple
   race when using `Dispatch.async` incorrectly. A more complete test
   suite for TSan's libdispatch support lives on the LLVM/compiler-rt
   side.

rdar://problem/49177535
@yln
Wire libdispatch build path through CMake
1f08ed4 
Wire libdispatch build path through CMake `build-script-impl ->
lit.site.cfg.in -> lit.cfg` instead of computing it in lit.cfg.
@yln yln force-pushed the tsan-positive-tests branch from b8f5715 to 1f08ed4 Compare June 10, 2019 21:28
@yln
Copy link
Contributor Author

yln commented Jun 10, 2019

@swift-ci Please test

@yln
Copy link
Contributor Author

yln commented Jun 10, 2019

Rebased on top of master.
@swift-ci Please test Linux platform

@yln
Copy link
Contributor Author

yln commented Jun 11, 2019

@compnerd Can you take a final quick look at this?

@gottesmm
Copy link
Contributor

LGTM if @compnerd is cool. I just pinged him.

@compnerd compnerd dismissed their stale review June 11, 2019 22:37

addressed

@yln yln merged commit 9298944 into master Jun 11, 2019
@yln yln deleted the tsan-positive-tests branch June 11, 2019 22:39
@finagolfin finagolfin mentioned this pull request Jan 27, 2020
Merged
@finagolfin finagolfin mentioned this pull request Mar 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrose-apple jrose-apple jrose-apple left review comments

@compnerd compnerd compnerd approved these changes

@gottesmm gottesmm Awaiting requested review from gottesmm

@kubamracek kubamracek Awaiting requested review from kubamracek

@devincoughlin devincoughlin Awaiting requested review from devincoughlin

@benlangmuir benlangmuir Awaiting requested review from benlangmuir

Assignees

@yln yln

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yln @swift-ci @davezarzycki @compnerd @gottesmm @jrose-apple