Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[macOS] Link __availability_version_check #45333

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Aug 31, 2023

Links in an implementation of _availability_version_check on macOS.
This is required due to an upstream compiler builtin (runtime) change
that marked this function as weakly-linked via __attribute__((weak import))
As such, no linking failure occurs when the function is unavailable at
link time. Since the symbol is no longer linked in, App Store review
blocks publishing due to relying on a private symbol.

By providing an implementation, the linker picks up our implementation,
which looks up symbol in question at runtime (via dlsym) on the first
invocation, caches it for later invocations, then invokes it. This is,
in fact, precisely what the original clang builtin implementation did.

Upstream clang change: https://reviews.llvm.org/D150397

Issue: flutter/flutter#133777

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@cbracken cbracken requested a review from zanderso August 31, 2023 17:32
@loic-sharma loic-sharma changed the title [macOS] Link __availaibility_version_check [macOS] Link __availability_version_check Aug 31, 2023
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

Thanks!

@cbracken cbracken force-pushed the availability-version-check branch from bd1f217 to aff0c2f Compare August 31, 2023 17:35
@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2023
@Hixie
Copy link
Contributor

Hixie commented Aug 31, 2023

Is "Calling dlsym with RTLD_DEFAULT can be very slow as all images in the process are searched for the symbol." (the reason for the upstream change) a concern for Flutter?

@cbracken cbracken removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2023
Links in an implementation of _availability_version_check on macOS.
This is required due to an upstream compiler builtin (runtime) change
that marked this function as weakly-linked via `__attribute__((weak import))`
As such, no linking failure occurs when the function is unavailable at
link time. Since the symbol is no longer linked in, App Store review
blocks publishing due to relying on a private symbol.

By providing an implementation, the linker picks up our implementation,
which looks up symbol in question at runtime (via dlsym) on the first
invocation, caches it for later invocations, then invokes it. This is,
in fact, precisely what the original clang builtin implementation did.

Upstream clang change: https://reviews.llvm.org/D150397

Issue: flutter/flutter#133777
@cbracken cbracken force-pushed the availability-version-check branch from aff0c2f to 11739f5 Compare August 31, 2023 18:11
@cbracken
Copy link
Member Author

Is "Calling dlsym with RTLD_DEFAULT can be very slow as all images in the process are searched for the symbol." (the reason for the upstream change) a concern for Flutter?

This is a single lookup that we cache on first call and never do again so the impact is low. Agreed that ideally we wouldn't even do that one lookup, but it is identical to the behaviour that we had all along -- i.e. not a regression, though not an improvement.

An alternative might be to go back to dealing with two clang toolchains and use Apple's Xcode toolchain rather than our own.

@zanderso
Copy link
Member

An alternative might be to go back to dealing with two clang toolchains and use Apple's Xcode toolchain rather than our own.

IMO, that approach would cause an unacceptable regression in CI build times.

@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2023
@auto-submit auto-submit bot merged commit c068806 into flutter:main Aug 31, 2023
@cbracken cbracken deleted the availability-version-check branch August 31, 2023 19:10
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 31, 2023
…133810)

flutter/engine@51090e3...ca513c9

2023-08-31 [email protected] Revert "Reland "Build iOS unittest target in unopt builds" (#44356)"" (flutter/engine#45346)
2023-08-31 [email protected] Lazily allocate RasterCacheItems only when caching is enabled (flutter/engine#45211)
2023-08-31 [email protected] Roll Skia from 8ff4fd208c26 to 5d08dadd2ef4 (3 revisions) (flutter/engine#45340)
2023-08-31 [email protected] Reland "Build iOS unittest target in unopt builds" (#44356)" (flutter/engine#44821)
2023-08-31 [email protected] Update comment const_finder.dart (flutter/engine#45180)
2023-08-31 [email protected] Replace an unnecessary util function with PostSync (flutter/engine#45190)
2023-08-31 [email protected] Roll Skia from cda0cfaadfd7 to 8ff4fd208c26 (3 revisions) (flutter/engine#45337)
2023-08-31 [email protected] Roll Dart SDK from 0cea73a8d3c3 to ac3bc9f6351a (4 revisions) (flutter/engine#45336)
2023-08-31 [email protected] [macOS] Link __availability_version_check (flutter/engine#45333)
2023-08-31 [email protected] Roll Skia from 8c05d5103d6b to cda0cfaadfd7 (3 revisions) (flutter/engine#45334)
2023-08-31 [email protected] Adds an --rbe option to tools/gn that works on Linux hosts (flutter/engine#45271)
2023-08-31 [email protected] Migrate VK calls of GrBackend* (flutter/engine#45325)
2023-08-31 [email protected] Roll buildroot (flutter/engine#45329)
2023-08-31 [email protected] Revert dl split (flutter/engine#45326)
2023-08-31 [email protected] Roll Skia from d113402de2ce to 8c05d5103d6b (4 revisions) (flutter/engine#45331)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@Hixie
Copy link
Contributor

Hixie commented Aug 31, 2023

Oh I'm definitely not saying we should have two toolchains. The implication of the upstream comment is that we could save some time on startup if we avoided the lookup. Hopefully we'll get that when we remove this workaround when Xcode updates (i.e. when everyone else does).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects: desktop autosubmit Merge PR when tree becomes green via auto submit App platform-ios platform-macos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants