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.

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

[tls] Remove support for pthread tls #31040

Merged
merged 38 commits into from Sep 23, 2022
Merged

Conversation

ctiller
Copy link
Member

@ctiller ctiller commented Sep 17, 2022

Alternative to #31030, #31036

It's looking likely that our pthread TLS implementation is no longer necessary.
@dennycd is going to confirm.

Assuming that's the case, let's finally delete this gunk.

@ctiller
Copy link
Member Author

ctiller commented Sep 18, 2022

@dennycd I'm having a heck of a time sorting out ios deployment versions in this PR so I'll probably ask for some help with that piece assuming we can move forward with this.

@dennycd
Copy link
Contributor

dennycd commented Sep 20, 2022

Updating w/ deployment target change removed. TLS was introduced by Apple from XCode 8.x and up

@dennycd
Copy link
Contributor

dennycd commented Sep 22, 2022

objc/iOS tests now all pass, a few summary notes below

  • grpc_objc_bazel_test: This is bazel-based run on newer MacOS 12.3 Monterey node with Xcode 13.3 (Clang 13.1.6). Our iOS configuration is (target device: iPhone 11, min deployment version: 9.0). This part is consistent w/ what we know about the TLS Apple support post Xcode 8/iOS 8.x. similarly for grpc_basictests_objc_ios

  • grpc_basictests_cpp_ios: Cocoapod based run on older MacOS 10.14 Mojave node with Xcode 11.3 (Clang 11.0). Our passing iOS configuration is (target device: iPhone 11, min deployment version: 10.0)

    • as tested, deployment versions below 10.0 appears not yet supporting TLS w/ thread_local

The discrepancy from # 2 likely come from running on MacOS Mojave node with older Xcode toolchain (XCode 11.3) which may not yet fully support TLS on all device targets.

Would recommend we upgrade grpc_basictests_cpp_ios to MacOS Monterey node in order to remove this configuration discrepancy (cl/475944915). ; )

Copy link
Contributor

@dennycd dennycd left a comment

Choose a reason for hiding this comment

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

iOS/ObjC part of this PR lgtm.

@ctiller ctiller merged commit f15ba1f into grpc:master Sep 23, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants