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

Added support for Apple Silicon (M1). #273

Closed
wants to merge 0 commits into from

Conversation

emeryberger
Copy link

Describe your changes
The existing compiler flags did not allow builds on Apple Silicon platforms (e.g., the M1 mini). This PR rectifies this situation, adding __arm64 detection and ensuring it is treated as a 64-bit platform.

Testing performed
Run with a test that uses the BDE library.

@osubboo
Copy link
Contributor

osubboo commented Jun 11, 2021

Noted. We'll look into it.

Copy link
Contributor

@AlisdairM AlisdairM left a comment

Choose a reason for hiding this comment

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

These lines need properly indenting to match surrounding #if blocks

@@ -292,7 +292,7 @@ extern "C" {
#include <time.h>
#endif

#if !(defined(BSLS_PLATFORM_OS_SOLARIS)) && !(defined(BSLS_PLATFORM_OS_AIX))
#if !(defined(BSLS_PLATFORM_OS_SOLARIS)) && !(defined(BSLS_PLATFORM_OS_AIX)) && !(defined(__arm64))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wordwrap with a line continuation for exceeding 79 columns

Choose a reason for hiding this comment

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

We shouldn't reference system macros directly but should be checking for the proper BSLS_PLATFORM macros.

Please change this (and the other preprocessor check below) to this:

#if    !(defined(BSLS_PLATFORM_OS_SOLARIS))                                   \
    && !(defined(BSLS_PLATFORM_OS_AIX))                                       \
    && !(defined(BSLS_PLATFORM_OS_DARWIN) && defined(BSLS_PLATFORM_CPU_ARM))

@@ -451,7 +451,7 @@ void SpinLock::doBackoff(int *count) {

inline
void SpinLock::pause() {
#if !(defined(BSLS_PLATFORM_OS_SOLARIS)) && !(defined(BSLS_PLATFORM_OS_AIX))
#if !(defined(BSLS_PLATFORM_OS_SOLARIS)) && !(defined(BSLS_PLATFORM_OS_AIX)) && !(defined(__arm64))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wordwrap with a line continuation for exceeding 79 columns

Copy link

@notadragon notadragon left a comment

Choose a reason for hiding this comment

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

Please change the macro guards in bsls_spinlock as I specified and then verify that this still builds on your M1. If it does we can get this PR landed.

Thanks.

@@ -292,7 +292,7 @@ extern "C" {
#include <time.h>
#endif

#if !(defined(BSLS_PLATFORM_OS_SOLARIS)) && !(defined(BSLS_PLATFORM_OS_AIX))
#if !(defined(BSLS_PLATFORM_OS_SOLARIS)) && !(defined(BSLS_PLATFORM_OS_AIX)) && !(defined(__arm64))

Choose a reason for hiding this comment

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

We shouldn't reference system macros directly but should be checking for the proper BSLS_PLATFORM macros.

Please change this (and the other preprocessor check below) to this:

#if    !(defined(BSLS_PLATFORM_OS_SOLARIS))                                   \
    && !(defined(BSLS_PLATFORM_OS_AIX))                                       \
    && !(defined(BSLS_PLATFORM_OS_DARWIN) && defined(BSLS_PLATFORM_CPU_ARM))

@mversche
Copy link
Contributor

Should this PR be closed in preference to #275?

@emeryberger
Copy link
Author

That does look like a more comprehensive PR than this one (I was just about to test).

@emeryberger
Copy link
Author

I updated to a more recent BDE and it no longer builds on the M1. I can work on it but if the other PR already works for a more recent version of BDE, it'd make more sense to merge that one. Let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants