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

Multiple fixes notably clock_gettime on Apple platforms #1725

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rcari
Copy link
Contributor

@rcari rcari commented Feb 25, 2022

The clock_gettime situation on Apple platforms is complex enough as it is.
Linking an incomplete implementation in replacement of the global symbol breaks
other libraries including libc++ which uses clock_gettime internally [1].
Depending on the approach taken by the libc++ the current implementation could
trigger infinite recursion calls as well.

Since the goal of the /portability/ is to make Folly portable, we should adapt
and use an approach which is compatible with Apple SDK development while
maintaining compatibility on other platforms.

This patch does that by adding a folly_ prefix to clock functions to avoid
clashing with the native symbol. Moreover it #define clock_getX folly_clock_getX
in order to maintain source compatibility.

As a side effect, this also addresses the redefinition issues that this [2]
other proposed fix solved.

[1] #1602
[2] #1724

@Orvid
Copy link
Contributor

Orvid commented Mar 8, 2022

The portability layer here is intended primarily to port other projects, rather than folly itself, so we try to maintain at least the appearance of the standard version of the API. For the issue you're running into here, would something similar to how fnctl functions are handled on windows work? (https://github.com/facebook/folly/blob/main/folly/portability/Fcntl.h) Basically declare them in a namespace and then add a global using namespace to make them appear in the right place.

The other changes look good though. Would it be possible to split those off into a different PR?

@rcari
Copy link
Contributor Author

rcari commented Mar 9, 2022

Hi @Orvid and thanks for your feedback!
I'm not sure I follow how Folly helps port other projects here. Such projects would have to #include <folly/portability/Time.h> to find these definitions but cannot use folly_clock_ instead of clock_? It looks like these other projects explicitly depend on Folly rather than standard library calls if they include Folly headers specifically.
My point in the PR description was that such library trying to achieve macOS portability should also follow Apple SDK development guidelines, or use these new Folly drop-in replacements.
In any case regarding what you mentioned with fcntl, I can give it a try but am wondering whether we won't run into symbol definition clashes...
In the meantime, I'll get the other fixes in another dedicated PR and keep you posted.

@rcari
Copy link
Contributor Author

rcari commented Aug 4, 2022

@Orvid after careful consideration, this would not work on Apple platforms because they define the symbols themselves whereas fcntl is not defined on WIN32 and thus defined entirely in Folly.
The only viable option here in my view would be to #define clock_gettime folly_clock_gettime, in the header and #undef clock_gettime in the implementation to actually lookup the right symbol from the SDK.

rcari added 3 commits August 5, 2022 17:01
The clock_gettime situation on Apple platforms is complex enough as it is.
Linking an incomplete implementation in replacement of the global symbol breaks
other libraries including libc++ which uses clock_gettime internally [1].
Depending on the approach taken by the libc++ the current implementation could
trigger infinite recursion calls as well.

Since the goal of the /portability/ is to make Folly portable, we should adapt
and use an approach which is compatible with Apple SDK development while
maintaining compatibility on other platforms.

This patch does that by adding a folly_ prefix to clock functions to avoid
clashing with the native symbol and #define clock_getX folly_clock_getX in order
to maintain source compatibility.

As a side effect, this also addresses the redefinition issues that this [2]
other proposed fix solved.

[1] facebook#1602
[2] facebook#1724
It looks like once you start using the FOLLY_NULLABLE or FOLLY_NONNULL macros in
a class, it's all or nothing or you get build warnings. This change addresses
that based on the assumptions of the underlying code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants