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

darwinssl: fix compiler warnings #1705

Closed
wants to merge 1 commit into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Jul 27, 2017

No description provided.

@bagder bagder added the TLS label Jul 27, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 75.289% when pulling 39008ee on bagder/darwinssl-warnings into 7551e55 on master.

@nickzman
Copy link
Member

I know those weak-link check lines were raising warnings, but I wonder if this change is the right thing to do. Because, according to ADC, the original code did the right thing: https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html (and scroll down to "Using Weakly Linked Symbols").

@bagder
Copy link
Member Author

bagder commented Jul 28, 2017

@nickzman I'm trying to get rid of the warnings and that way was suggested by clang itself, so if that is the wrong way, can we figure out a better approach? We want warning-free builds, in particular right now since I have #1706 in progress to add darwinssl-builds to travis.

@nickzman
Copy link
Member

Yeah, I just tried building this branch with the compiler option "-mmacosx-version-min=10.6" and got a number of build errors:

vtls/darwinssl.c:857:7: error: 'SecCertificateCopyLongDescription' is only
available on macOS 10.7 or newer [-Werror,-Wunguarded-availability]
if(&SecCertificateCopyLongDescription != NULL)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Security.framework/Headers/SecCertificate.h:488:13: note:
'SecCertificateCopyLongDescription' has been explicitly marked partial
here
CFStringRef SecCertificateCopyLongDescription(CFAllocatorRef __nullable ...
^
vtls/darwinssl.c:857:7: note: enclose 'SecCertificateCopyLongDescription' in an
@available check to silence this warning
if(&SecCertificateCopyLongDescription != NULL)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think wrapping the code in "@available" as the message above recommends would be a better fix. The catch is, it's an extension that was added to LLVM 5.0, so if we use it, we'll break building with GCC on older macOS releases. (And I'm okay with that, actually.)

@bagder
Copy link
Member Author

bagder commented Jul 29, 2017

Ok, so my take was plain wrong. Thanks for clarifying that @nickzman !

I figure a fix based on @available should be fine. I would presume we can even make an #ifdef thing to make older compilers without support for that construct to not use it.

Would you be able to write up a take on that, @nickzman ?

@nickzman
Copy link
Member

nickzman commented Aug 2, 2017

I started on it, but it appears that the C equivalent function (__builtin_available()) only builds correctly in the Xcode 9 beta, which means we'll solve the build warnings, but blow away a lot more backward compatibility than I thought we'd blow away. I'll continue, but when I'm done, it probably shouldn't be merged until after Xcode 9 launches this fall.

@bagder
Copy link
Member Author

bagder commented Aug 2, 2017

I'll experiment with silencing them by simply ignoring that specific error. I'll close this and open a new PR with that approach.

@bagder bagder closed this Aug 2, 2017
@bagder bagder deleted the bagder/darwinssl-warnings branch August 2, 2017 11:58
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants