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.

Sign up for GitHub

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

Jump to bottom

Fix openNoInt building error after android ndk r21 #1593

Closed
wants to merge 1 commit into from
Closed

Fix openNoInt building error after android ndk r21 #1593

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions folly/FileUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,26 @@ namespace folly {
using namespace fileutil_detail;

int openNoInt(const char* name, int flags, mode_t mode) {
#if defined(__ANDROID__)
// NDK bionic with FORTIFY has this definition:
// https://android.googlesource.com/platform/bionic/+/9349b9e51b/libc/include/bits/fortify/fcntl.h
// ```
// __BIONIC_ERROR_FUNCTION_VISIBILITY
// int open(const char* pathname, int flags, mode_t modes, ...) __overloadable
// __errorattr(__open_too_many_args_error);
// ```
// This is originally to prevent open() with incorrect parameters.
//
// However, combined with folly wrapNotInt, template deduction will fail.
// In this case, we create a custom Open lambda to bypass the error.
// The solution is referenced from
// https://github.com/llvm/llvm-project/commit/0a0e411204a2baa520fd73a8d69b664f98b428ba
//
auto Open = [&]() { return open(name, flags, mode); };
Copy link
Contributor

@yfeldblum yfeldblum Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Let's write this as:

auto openWrap = +[](const char* name_, int flags_, mode_t mode_) {
  return open(name_, flags_, mode_);
}
auto openFunc = kIsAndroid ? openWrap : open;
return int(wrapNoInt(openFunc, name, flags, mode));

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be simpler to just use the lambda in all cases, and don't bother checking kIsAndroid. The use of the lambda doesn't appear to affect the code generated by either gcc or clang in optimized builds.

I think we can just say

auto openWrapper = [&] { return open(name, flags, mode); };
return int(wrapNoInt(openWrapper));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review.
should i revise changes in this pr, as imported to facebook internal phabricator already?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to revise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the import blocked due to some reason ? I see it has been blocked for two days.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is merged internally. The syncing from the internal repo to github is not working at the moment, but once it is working again this change will be merged on github as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

When this synced to github. Can you draft a new release for this ? react-native has been stuck for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

@simpkins Looks the import is stuck.

Copy link
Contributor

Choose a reason for hiding this comment

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

@simpkins ping

return int(wrapNoInt(Open));
#else
return int(wrapNoInt(open, name, flags, mode));
#endif
}

static int filterCloseReturn(int r) {
Expand Down