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.

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

ci: build LLVM with thread support on Windows #3130

Merged
merged 1 commit into from
Mar 7, 2023
Merged

ci: build LLVM with thread support on Windows #3130

merged 1 commit into from
Mar 7, 2023

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Sep 3, 2022

This should fix a number of concurrency/threading issues.

Ok, last attempt (in a new PR because I can't reopen #2433).

@aykevl
Copy link
Member Author

aykevl commented Sep 3, 2022 •
edited
Loading

Ok, this still doesn't work. I don't know why. I will have to debug this locally at some point.

(Note for the future: the "Test TinyGo" step seems to be hanging).

@aykevl aykevl closed this Sep 3, 2022
@aykevl
Copy link
Member Author

aykevl commented Feb 3, 2023

Tested locally with LLVM 15, which appears to work well?? Let's try it again.

@aykevl aykevl reopened this Feb 3, 2023
@aykevl
Copy link
Member Author

aykevl commented Feb 3, 2023

Why oh why doesn't this work??

@aykevl
Copy link
Member Author

aykevl commented Mar 6, 2023 •
edited
Loading

Trying once more. This time my plan is as follows:

  • figure out which test hangs.
  • run only that test, to check it still hangs
  • build a test binary, download it, and debug the issue locally

Updates:

  1. it's tinygo.test
  2. It's a deadlock. When I run it locally, I can see that 4 extra processes are created, but they simply don't exit. They just continue running. Presumably, these processes are the separate clang / ld.lld processes.
  3. The processes that hang are all ld.lld processes (e.g. tinygo.test.exe ld.lld -m i386pep [...]).
  4. It's also hanging in the ELF linker, it's not just the COFF linker.
  5. 🎉 --threads=1 seems to be a workaround, at least for ELF 🦄

@aykevl aykevl mentioned this pull request Mar 6, 2023
Closed
@aykevl aykevl force-pushed the windows-llvm-threads branch 6 times, most recently from 040ce18 to 8b9deb1 Compare March 7, 2023 20:03
@aykevl
ci: build LLVM with thread support on Windows
f2303d1 
This should fix a number of concurrency/threading issues.

I had to force-disable concurrency in the linker using a hack. I'm not
entirely sure what the cause is, possibly the MinGW version (version 12
appears to work for me, while version 11 as used on the GitHub runner
image seems to be broken).
There are a few ways to fix this in a better way:
  * Fix the underlying cause (possibly by upgrading to MinGW-w64 12).
  * Add the `--threads` flag to the LLD MinGW linker, so we can use a
    regular parameter instead of this hack.
@aykevl
Copy link
Member Author

aykevl commented Mar 7, 2023

This passes all tests that are affected by this change, so to me it seems like it's ready for merge.

@deadprogram
Copy link
Member

Do you also need to remove #3525 for this?

@aykevl
Copy link
Member Author

aykevl commented Mar 7, 2023 •
edited
Loading

Do you also need to remove #3525 for this?

Eventually, yes. But let's first see whether this PR improves the situation (because #3525 didn't seem to do a whole lot).
Regardless of #3525, I think we need this change to not misuse the LLVM APIs (using goroutines while thread support is disabled).

@deadprogram
Copy link
Member

Makes sense to me, merging!

@deadprogram deadprogram merged commit 34ba0c1 into dev Mar 7, 2023
@deadprogram deadprogram deleted the windows-llvm-threads branch March 7, 2023 21:38
@aykevl
Copy link
Member Author

aykevl commented Mar 7, 2023

It took me just over a year (and many hours debugging this), but we are finally building LLVM with thread support.

aykevl added a commit that referenced this pull request May 13, 2023
@aykevl
windows: re-enable parallelism
9d89f79 
This reverts #3525, because
that change didn't seem to stop the CI failures we have been seeing.
Instead, I've added thread support in
#3130 which IIRC fixed most of
the CI crashes.

Re-enabling parallelism should improve the performance of TinyGo a bit
on Windows.
@aykevl aykevl mentioned this pull request May 13, 2023
Merged
deadprogram pushed a commit that referenced this pull request May 16, 2023
@aykevl @deadprogram
windows: re-enable parallelism
af76c80 
This reverts #3525, because
that change didn't seem to stop the CI failures we have been seeing.
Instead, I've added thread support in
#3130 which IIRC fixed most of
the CI crashes.

Re-enabling parallelism should improve the performance of TinyGo a bit
on Windows.
LiuJiazheng pushed a commit to LiuJiazheng/tinygo that referenced this pull request Aug 20, 2023
@aykevl @LiuJiazheng
windows: re-enable parallelism
92c5bca 
This reverts tinygo-org#3525, because
that change didn't seem to stop the CI failures we have been seeing.
Instead, I've added thread support in
tinygo-org#3130 which IIRC fixed most of
the CI crashes.

Re-enabling parallelism should improve the performance of TinyGo a bit
on Windows.
deadprogram pushed a commit that referenced this pull request Sep 17, 2023
@aykevl @deadprogram
windows: re-enable parallelism
27be11d 
This reverts #3525, because
that change didn't seem to stop the CI failures we have been seeing.
Instead, I've added thread support in
#3130 which IIRC fixed most of
the CI crashes.

Re-enabling parallelism should improve the performance of TinyGo a bit
on Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aykevl @deadprogram