Skip to content

Added support for Apple Silicon (M1).#273

Closed
emeryberger wants to merge 0 commit into
bloomberg:masterfrom
emeryberger:master
Closed

Added support for Apple Silicon (M1).#273
emeryberger wants to merge 0 commit into
bloomberg:masterfrom
emeryberger:master

Conversation

@emeryberger

Copy link
Copy Markdown

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

osubboo commented Jun 11, 2021

Copy link
Copy Markdown
Contributor

Noted. We'll look into it.

@AlisdairM AlisdairM left a comment

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.

These lines need properly indenting to match surrounding #if blocks

Comment thread groups/bsl/bsls/bsls_spinlock.h Outdated
#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
Copy Markdown
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
Copy Markdown

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))

Comment thread groups/bsl/bsls/bsls_spinlock.h Outdated
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
Copy Markdown
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

@notadragon notadragon left a comment

Copy link
Copy Markdown

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.

Comment thread groups/bsl/bsls/bsls_spinlock.h Outdated
#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
Copy Markdown

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
Copy Markdown
Contributor

Should this PR be closed in preference to #275?

@emeryberger

Copy link
Copy Markdown
Author

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

@emeryberger

Copy link
Copy Markdown
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