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

Fix usage of unavailable weak symbol clock_gettime() on Apple platform. #3048

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dmitrykos
Contributor

dmitrykos commented Sep 25, 2018

Latest Apple SDK declares clock_gettime() and it is available during compile time but it may not always be available during run time - weak symbol. E.g. code compiles but later will crash if clock_gettime() is used on unsupported version of Apple OS.

clock_gettime() is supported since these OS versions: macOS 10.12, iOS 10, tvOS 10, watchOS 3.

This fix uses __builtin_available() Clang's extension which tests OS version and does not allow to use clock_gettime() on unsupported OS version.

There is only small overhead on Apple OS due to have_clock_gettime variable check but it makes binary portable (clock_gettime() will not be used on unsupported OS version) and ready for usage of high-resolution monotonic clock on Apple OS (clock_gettime() will be used on supported OS version).

Other platforms are unaffected by this implementation due to HAVE_CLOCK_GETTIME_CHECK define used in place.

Show resolved Hide resolved lib/timeval.c
Show resolved Hide resolved lib/timeval.c Outdated
@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Sep 26, 2018

Commited change from int to bool type and also would like to clarify regarding why static bool construction is used: the reason is that __builtin_available() can not be used in combination with other checks inside if.

@bagder bagder requested a review from nickzman Sep 26, 2018

@nickzman

I still don't like this. Is there any reason why we can't use traditional weak-linking here instead of __builtin_available()? __builtin_available() won't work with compilers other than LLVM.

If we can't use traditional weak-linking, then why don't we use the HAVE_BUILTIN_AVAILABLE preprocessor macro instead of making a new one?

Show resolved Hide resolved lib/timeval.c

@dmitrykos dmitrykos closed this Sep 27, 2018

@dmitrykos dmitrykos reopened this Sep 27, 2018

@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Sep 27, 2018

can't use traditional weak-linking here instead of __builtin_available()?

If you mean by weak-linking the dlsym operation then it is undefined whether it will load clock_gettime symbol or its stub with internal assertion, to my view. It depends on Apple's implementation which they can change anytime at their sole discretion. Apple SDK is relying on LLVM/Clang and its __builtin_available functionality thus for a robust, always working binaries it makes sense to use the same functionality.

we can't just tell them off, especially when the rest of the library builds just fine with GCC.

Proposed changes do not affect GCC users in any sense, so there is no limitation, code compiles and works successfully. If GCC user defines HAVE_CLOCK_GETTIME_MONOTONIC then clock_gettime will be used without any problem.

But the main problem with GCC on Apple platform - it is depreciated by Apple and thus it does not make sense to try to use GCC + recent Apple SDK based on LLVM/Clang to compile binaries for production. I do not think anybody will do that. And older Apple SDK, which is using/compatible with GCC. does not have clock_gettime defined anyway (HAVE_CLOCK_GETTIME_MONOTONIC will be undefined). So, all is ok with GCC users to my view either.

To avoid confusion with public defines of HAVE_XXX style I propose to rename HAVE_CLOCK_GETTIME_CHECK to _USE_CLOCK_GETTIME_CHECK.

@bagder

This comment has been minimized.

Member

bagder commented Sep 27, 2018

But since the variable is always updated by the if(), why does it need to be static? Surely removing static from it will keep it working just as good and just remove potential problems?

make variable local, simplified builtin detection
make variable local, simplified __builtin_available  detection with already existing HAVE_BUILTIN_AVAILABLE define
@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Sep 27, 2018

@bagder, agree that variable may be local. Also replaced check for __builtin_available with existing HAVE_BUILTIN_AVAILABLE define (did not notice that it is used by curl already, not defined in CMake configs?) as advised by @nickzman. Applied necessary changes.

I used HAVE_BUILTIN_AVAILABLE in the way darwinssl.c uses it, but connect.c uses HAVE_BUILTIN_AVAILABLE as #if defined(HAVE_BUILTIN_AVAILABLE) both checks will work unless HAVE_BUILTIN_AVAILABLE is defined as

#define HAVE_BUILTIN_AVAILABLE 0
limit clock_gettime check to only Apple OS
limit clock_gettime check to only Apple OS because LLVM/Clang defines __builtin_available() on other platforms too (for example Android, NDK r17b)
@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Sep 27, 2018

It appears that __builtin_available is also available on other platforms in LLVM/Clang, for example Android (NDK r17b), thus limited check to only Apple OS.

@nickzman

This comment has been minimized.

Collaborator

nickzman commented Oct 1, 2018

I suppose this is fine then. By "traditional" weak linking, I meant the way it was done prior to __builtin_available(), which was to check at run-time if the symbol was defined or not. The darwinssl.c code still does this for backward compatibility with older compilers, and it hasn't caused any problems. But using __builtin_available() will solve this for the vast majority of users.

@bagder

This comment has been minimized.

Member

bagder commented Oct 5, 2018

Thanks!

@bagder bagder closed this in 0b19ef1 Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment