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

bn_ppc.c: Fix build failure on AIX with XLC/XLCLANG #17497

Closed
wants to merge 1 commit into from
Closed

bn_ppc.c: Fix build failure on AIX with XLC/XLCLANG #17497

wants to merge 1 commit into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Jan 13, 2022

These compilers define _ARCH_PPC64 for 32 bit builds
so we cannot depend solely on this define to identify
32 bit build.

Fixes #17087

@t8m
bn_ppc.c: Fix build failure on AIX with XLC/XLCLANG
dce24d7 
These compilers define _ARCH_PPC64 for 32 bit builds
so we cannot depend solely on this define to identify
32 bit build.

Fixes openssl#17087
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Jan 13, 2022
mattcaswell
mattcaswell reviewed Jan 13, 2022
View reviewed changes
crypto/bn/bn_ppc.c
@@ -40,7 +40,7 @@ int bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp,
* no opportunity to figure it out...
*/

#if defined(_ARCH_PPC64)
#if defined(_ARCH_PPC64) && !defined(__ILP32__)
Copy link
Member

Choose a reason for hiding this comment

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

There are other instances of the use of this macro which look suspect to me, e.g. see crypto/sha/sha512.c

Also bn_local.h does this correctly, but in a different way:

#  elif defined(_ARCH_PPC64) && defined(SIXTY_FOUR_BIT_LONG)

Should we be consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO that is a different condition. I am not sure about them being equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the other instances, I am not sure about them, I suppose they are either working for the platform from #17087 correctly or not compiled in for other reasons.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. We can run with this as is - but I'm not entirely convinced there isn't another issue lurking somewhere.

@t8m t8m mentioned this pull request Jan 13, 2022
Closed
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 14, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Jan 15, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jan 15, 2022
openssl-machine pushed a commit that referenced this pull request Jan 17, 2022
@t8m
bn_ppc.c: Fix build failure on AIX with XLC/XLCLANG
cfbb5fc 
These compilers define _ARCH_PPC64 for 32 bit builds
so we cannot depend solely on this define to identify
32 bit build.

Fixes #17087

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17497)
openssl-machine pushed a commit that referenced this pull request Jan 17, 2022
@t8m
bn_ppc.c: Fix build failure on AIX with XLC/XLCLANG
454358b 
These compilers define _ARCH_PPC64 for 32 bit builds
so we cannot depend solely on this define to identify
32 bit build.

Fixes #17087

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17497)

(cherry picked from commit cfbb5fc)
@t8m
Copy link
Member Author

t8m commented Jan 17, 2022

Merged to master and 3.0 branches. Thank you for the review.

@t8m t8m closed this Jan 17, 2022
@t8m t8m mentioned this pull request Jul 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[3.0.0] Build failures on AIX 7.2 XLC/XLCLANG 16.1 32bit
3 participants
@t8m @openssl-machine @mattcaswell