-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
aes_platform.h: add missing ppc64 define #23245
Conversation
Hmm, actually we do not use __ppc64__anywhere. Is it possible that _ARCH_PPC is defined even on the 64 bit PowerPC? If not, we use _ARCH_PPC64 elsewhere in the code instead. Also please note there are other places where we use only |
@t8m The issue is that neither of these three is defined on Darwin ppc64. What will work is either Of course, it is not needed to do in every file, if there are numerous instances; what can be done then is a single OpenSSL-internal define in a general header, something like Notice, by the way, GCC does use |
Please use |
@t8m Sure, will do. Do we still retain |
Yeah, |
Current PowerPC-related defines omit Darwin ppc64 case. Use __POWERPC__ in place of __ppc__ + __ppc64__ Fixes openssl#23220 CLA: trivial
@t8m Done as advised. |
I assume you've tried to build it on the Power Mac and the tests passed? |
@t8m Yes originally, let me run again in the form to which we arrived, and confirm. Will be done in a couple of hours. |
@t8m YES, with two instances changed to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with CLA: trivial
There are a few instances of |
@tom-cosgrove-arm Hmm, from a first glance it is needed (unless the file itself is excluded for macOS for a good reason), but the code is non-trivial and has to be specifically tested both for ppc and ppc64, since apparently it was never being compiled on macOS PowerPC. Allow me to do that. |
Maybe this should be a separate PR then? |
@t8m Fair enough, that does not need to be here. Then this one should be good to merge. |
Ping @openssl/committers for second approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a PPC expert (it's a long time since I booted that G4 Mac mini of mine), but looks good to me.
Okay with trivial
This pull request is ready to merge |
Merged to the master branch. Thank you for your contribution. |
Thank you! |
Fixes: #23220
@t8m Could you please review this?
P. S. We could get the same effect for macOS by adding
__POWERPC__
, but that gonna also include BeOS, and I have no idea if related code gonna work there or not. Perhaps__ppc64__
is a better pick.