Skip to content

Fixes for older macOS and atomic builtins detection#1907

Open
barracuda156 wants to merge 2 commits into
facebook:mainfrom
barracuda156:darwin
Open

Fixes for older macOS and atomic builtins detection#1907
barracuda156 wants to merge 2 commits into
facebook:mainfrom
barracuda156:darwin

Conversation

@barracuda156

Copy link
Copy Markdown

With these fixes folly builds on older macOS, including 10.5.8 and 10.6 on PowerPC. I have put PPC-related ones into a separate commit.

One non-macOS-specific fix is related to atomic builtins detection. Current folly configure code does not work correctly: even when atomics requires linking to libatomic, the test still passes, as if it is not needed: Performing Test FOLLY_CPP_ATOMIC_BUILTIN - Success; sure enough, the build fails at linking stage.
I borrowed the check from here, where it has been tested on macOS and Linux 32-bit: uxlfoundation/oneTBB#987

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@barracuda156

Copy link
Copy Markdown
Author

Those Fedora checks failing in 0 sec cannot be due to the code changes, I guess.

@barracuda156

Copy link
Copy Markdown
Author

I will see why it fails now, allow me a bit of time. On Mac it should build in fact, since it builds on Macports buildbots.

@barracuda156

Copy link
Copy Markdown
Author

@Orvid Referring to your reply earlier: #1835 (comment)
Could you please take a look? Will be greatly appreciated.

P. S. Non-Mac and non-PPC test result in checks should be irrelevant (I looked at some, and appears they are). I could not have broken anything on new MacOS too, I believe.

Comment thread folly/File.cpp Outdated
Comment thread folly/SocketAddress.cpp Outdated
base64URLDecodeSWAR};
}
#endif
#if defined(__POWERPC__) // PowerPC BE

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this get defined for PPC64 LE, or only 32-bit PPC BE?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also would prefer to check for FOLLY_PPC if this is only ppc 32-bit be.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@Orvid __POWERPC__ includes __ppc__ and __ppc64__ – only Big endian, and for practical purposes only Darwin. (I think it was also used on BeOS.)

I generally try to be specific, since I have no way to test on *BSD, AIX or Linux, which could also use ppc32. Perhaps FOLLY_PPC_BE may do, since we want to have macOS in one define, but do not include ppc64le (well, not in general case).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added a define for non-Apple PPC Big endian. Hope it is correct.

Comment thread folly/io/async/AsyncUDPSocket.cpp Outdated
@barracuda156 barracuda156 force-pushed the darwin branch 2 times, most recently from 87f4e95 to f4c49e1 Compare January 30, 2023 13:10
@barracuda156

Copy link
Copy Markdown
Author

@Orvid Please take a look. I updated the patched, taking into account your recommendations, and verified that folly latest release builds with them fine on ppc32.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

barracuda156 referenced this pull request Jan 12, 2024
Summary:
RISC-V is a growing platform, and Folly and commonly used dependency. We want to make sure that any projects that relies on Folly can be ported to RISC-V.

I'm verifying that everything builds and run correctly by cross-compiling it locally and passing the test suite using QEMU.

Pull Request resolved: #2104

Reviewed By: yfeldblum

Differential Revision: D51691588

Pulled By: Orvid

fbshipit-source-id: 9c4ae6718ffbf6b55432bf6e1ffba06311c7d345
facebook-github-bot pushed a commit that referenced this pull request Sep 5, 2024
Summary:
Some platforms need libatomic linked for sub-word operations while others need it for super-word operations. Let's make sure we test for all.

Relates to #1907

Pull Request resolved: #2126

Reviewed By: Gownta

Differential Revision: D52975209

Pulled By: yfeldblum

fbshipit-source-id: 2c8781f817d6cc59fb8c74c1d13812e584187b47
facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Sep 11, 2024
Summary:
Some platforms need libatomic linked for sub-word operations while others need it for super-word operations. Let's make sure we test for all.

Relates to facebook/folly#1907

X-link: facebook/folly#2126

Reviewed By: Gownta

Differential Revision: D52975209

Pulled By: yfeldblum

fbshipit-source-id: 2c8781f817d6cc59fb8c74c1d13812e584187b47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants