Skip to content

Ensure FreeType compiles with pthreads support when necessary. #16546

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

Merged
merged 3 commits into from
Mar 29, 2022
Merged

Ensure FreeType compiles with pthreads support when necessary. #16546

merged 3 commits into from
Mar 29, 2022

Conversation

Asaaj
Copy link
Contributor

@Asaaj Asaaj commented Mar 21, 2022

This solves an error similar to those discussed in #8503, in which a program which needed both pthreads and freetype was failing to compile with this error:

wasm-ld: error: 'atomics' feature is disallowed by sfnt.c.o, so --shared-memory must not be used

Making this change locally solved the issue. Solved in the same style as #8960.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Can you modify the test_freetype test in test_other.py. Presumably adding -sUSE_PTHREADS to that test will cause it to fail (without this patch).

@Asaaj
Copy link
Contributor Author

Asaaj commented Mar 22, 2022

I verified the new test does fail without these changes

# Verify that freetype supports compilation requiring pthreads
self.emcc(test_file('freetype_test.c'), ['-sUSE_PTHREADS', '-sUSE_FREETYPE'], output_filename='a.out.js')
except Exception:
self.fail("Compiling freetype with pthreads enabled failed.")
Copy link
Collaborator

@sbc100 sbc100 Mar 22, 2022

Choose a reason for hiding this comment

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

You don't need the try catch. Just the one self.emcc line is enough (and the comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I amended in the fork to keep it cleaner

@sbc100
Copy link
Collaborator

sbc100 commented Mar 22, 2022

Actually there is one more detail we forgot to consider here. The other ports that have multi-threaded variants use an alternative name (i.e. the -mt) suffice. See tools/ports/icu.py for an example.

We have two options here I think:

  1. Introduce a -mt variant filename when building with pthreads
  2. Always build with pthreads assuming that the runtime/browser will accept the atomic access even in single threaded mode (see Allowing atomic instructions with unshared memory WebAssembly/threads#144).

The later option would keep the code simpler. If (2) works and we can pass both the theaded and non-threaded tests then I think it would be preferable. All you would need to do it simply always pass -sUSE_PTHREADS (actually can you pass the standard -pthread flag instead?)

@Asaaj
Copy link
Contributor Author

Asaaj commented Mar 23, 2022

In my current configuration, I am unable to run the other tests in general. I get a huge number of exceptions, and it's unable to import acorn. I assume I can follow a guide to solve that, but would this change be something you'd be able to verify the correctness of more easily? If not, I won't be able to get to it until next week

@sbc100
Copy link
Collaborator

sbc100 commented Mar 23, 2022

In my current configuration, I am unable to run the other tests in general. I get a huge number of exceptions, and it's unable to import acorn. I assume I can follow a guide to solve that, but would this change be something you'd be able to verify the correctness of more easily? If not, I won't be able to get to it until next week

Maybe you didn't run npm install?

Expects the runtime will support atomic access, even in single-threaded
mode. See WebAssembly/threads#144 for more.
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.

2 participants