[Swift in WebKit] The bmalloc module map is malformed (must include pas_lock.h) #51836
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
rr-codes
added
the
WebKit Misc.
|
EWS run on previous version of this PR (hash 57677d5) |
rr-codes
requested review from
aprotyas,
hortont424,
lilyspiniolas,
pxlcoder and
whsieh
rr-codes
force-pushed
the
eng/Swift-in-WebKit-The-bmalloc-module-map-is-malformed-must-include-pas_lock-h
branch
from
57677d5 to
61ff0aa
Compare
|
EWS run on previous version of this PR (hash 61ff0aa) |
adetaylor
reviewed
Oct 6, 2025
| os_unfair_lock_lock_with_options_inline(&lock->lock, options); | ||
| #else | ||
| PAS_UNUSED_PARAM(options); | ||
| PAS_ASSERT_NOT_REACHED(); |
There was a problem hiding this comment.
When I originally put together the bmalloc modulemap, I assumed (perhaps incorrectly) that we needed to maintain the property of a compile-time failure if OS_UNFAIR_LOCK_INLINE was not defined. I'm glad if that's no longer the case...
There was a problem hiding this comment.
Yes, that would be ideal, however it is impossible to maintain that while also having bmalloc be properly modularized that, so the solution described in a PR is a reasonable alternative.
adetaylor
reviewed
Oct 6, 2025
| pas_log("Thread %p Trylocking lock %p\n", pthread_self(), lock); | ||
| #if PAS_USE_ULOCK_SPI && (!defined(OS_UNFAIR_LOCK_INLINE) || !OS_UNFAIR_LOCK_INLINE) | ||
| result = false; | ||
| PAS_ASSERT_NOT_REACHED(); |
There was a problem hiding this comment.
Your commit message says this is a release assert. PAS_ENABLE_ASSERT seems a bit more tricksy than that, it seems to depend on the platform. I assume you're aware, just checking.
There was a problem hiding this comment.
Yes, Adrian is correct. Use this instead: PAS_ASSERT_IF(true, !"Should not be reached").
MenloDorian
approved these changes
Oct 6, 2025
MenloDorian
added
the
merging-blocked
rr-codes
removed
the
merging-blocked
rr-codes
force-pushed
the
eng/Swift-in-WebKit-The-bmalloc-module-map-is-malformed-must-include-pas_lock-h
branch
from
61ff0aa to
d26cea3
Compare
|
EWS run on previous version of this PR (hash d26cea3) |
rr-codes
added
merge-queue
rr-codes
force-pushed
the
eng/Swift-in-WebKit-The-bmalloc-module-map-is-malformed-must-include-pas_lock-h
branch
from
d26cea3 to
88c07de
Compare
|
EWS run on current version of this PR (hash 88c07de) |
…as_lock.h) https://bugs.webkit.org/show_bug.cgi?id=300198 rdar://161993222 Reviewed by Mark Lam. Currently, (in Cocoa configurations at least) bmalloc depends on its client setting `OS_UNFAIR_LOCK_INLINE` to `1`; otherwise, a compilation error happens. However, modules require that libraries/frameworks are usable without any explicit configuration set. Therefore, bmalloc must at least be able to compile all its translation units regardless of the value of `OS_UNFAIR_LOCK_INLINE`. To workaround this, the `pas_lock.h` file was initially excluded from the module map. However, this is invalid as it results in modular headers then including non-modular headers, which is a violation. This has not been a problem so far because (a) there is no automatic module verification for libraries and (b) there is yet a Swift/Cxx interop include path in WebKit or WebCore that depends on a file that uses `bmalloc`. Manually performing module verification does reveal this issue, which is a violation and needs to be remedied. To fix, the `#error` directive must be removed so that bmalloc can be built regardless of the value of `OS_UNFAIR_LOCK_INLINE`. Consequently, some more compilation conditions have to be added to the file to account for configurations where `OS_UNFAIR_LOCK_INLINE` is `0` since the relevant locking APIs from the system are unavailable. In actuality, these paths where `OS_UNFAIR_LOCK_INLINE` is `0` will never actually be executed; they just need to be able to compile. Since this is technically a slight change of behavior, there are several safe guards added: 1. There are currently no Cocoa bmalloc clients that set `OS_UNFAIR_LOCK_INLINE` to `0` or leave it undefined 2. If, for some reason, one of the existing clients changed the value from `1` to `0`, a compiler diagnostic message will be emitted with the same message as the error would have given. 3. The invalid branches are limited in scope, and immediately cause a release assertion if triggered. One alternative considered was to wrap every header and translation unit in bmalloc with a compile time guard; this was decided against because although this would technically not be a change in behavior for bmalloc, the changes would be quite extensive and intrusive. Additionally, this would just move the issue up one dependency to the client of bmalloc anyways. * Source/WebKit/Configurations/WebKit.xcconfig: * Source/bmalloc/Configurations/module.modulemap: * Source/bmalloc/libpas/src/libpas/pas_lock.h: (pas_lock_lock): (pas_lock_try_lock): (pas_lock_unlock): Canonical link: https://commits.webkit.org/301104@main
webkit-commit-queue
force-pushed
the
eng/Swift-in-WebKit-The-bmalloc-module-map-is-malformed-must-include-pas_lock-h
branch
from
88c07de to
3ecd1b1
Compare
|
Committed 301104@main (3ecd1b1): https://commits.webkit.org/301104@main Reviewed commits have been landed. Closing PR #51836 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
3ecd1b1
88c07de
🛠 win🧪 wpe-wk2🧪 win-tests🧪 ios-wk2🧪 api-wpe🧪 ios-wk2-wpt🧪 api-ios🧪 mac-AS-debug-wk2🧪 gtk-wk2🧪 api-gtk🧪 vision-wk2🧪 mac-intel-wk2🛠 tv-sim🧪 jsc-armv7-tests🛠 watch🛠 watch-sim