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

Updating ifdefs to account for xlclang compiler frontend on AIX. The … #18892

Closed
wants to merge 1 commit into from
Closed

Updating ifdefs to account for xlclang compiler frontend on AIX. The … #18892

wants to merge 1 commit into from

Conversation

robmcgee-mag
Copy link
Contributor

…fallback DEP works fine there. XLC should be unaffected.

CLA: trivial

Checklist
  • documentation is added or updated
  • tests are added or updated

@robmcgee-mag
Updating ifdefs to account for xlclang compiler frontend on AIX. The …
88a6f8c 
…fallback DEP works fine there. XLC should be unaffected.

CLA: trivial
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 27, 2022
@robmcgee-mag robmcgee-mag mentioned this pull request Jul 27, 2022
Closed
2 tasks
@t8m
Copy link
Member

t8m commented Jul 28, 2022

Not sure why the cla-check did not recognize the CLA: trivial. Anyway, that shouldn't be a problem.

@t8m t8m added branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' branch: 3.0 Merge to openssl-3.0 branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: bug The issue/pr is/fixes a bug and removed hold: cla required The contributor needs to submit a license agreement labels Jul 28, 2022
@t8m
Copy link
Member

t8m commented Jul 28, 2022

I am OK with CLA: trivial for this.

tmshort
tmshort reviewed Jul 28, 2022
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

…fallback DEP works fine there. XLC should be unaffected.

So, if both __GNUC__ and _AIX are defined, then the final #else is hit.

Is this acceptable? (Is that what you meant?)

@robmcgee-mag
Copy link
Contributor Author

robmcgee-mag commented Jul 28, 2022 •
edited
Loading

Correct. __GNUC__ is only defined with _AIX when running the new xlclang compiler frontend that supports C++11 (poorly at that). The older compiler, XLC, should still use the _init()/_cleanup() functions in the #ifdef.

tmshort
tmshort approved these changes Jul 28, 2022
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

LGTM based on the explanation.

@tmshort tmshort 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 Jul 28, 2022
@tmshort
Copy link
Contributor

tmshort commented Jul 28, 2022

Pending CI, of course!

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jul 28, 2022
@robmcgee-mag
Copy link
Contributor Author

robmcgee-mag commented Jul 29, 2022 •
edited
Loading

I didn't run into this issue during my own build using 3.0.5, although I did build 32bit with
CC=xlclang CXX=xlclang++ /usr/bin/perl ./Configure aix-cc shared enable-fips -DBROKEN_CLANG_ATOMICS no-asm

@paulidale
Copy link
Contributor

Okay with trivial

@paulidale
Copy link
Contributor

The CLA: trivial line isn't being found. Is there a new line at the end?

@robmcgee-mag
Copy link
Contributor Author

There is a newline after it.
image

@t8m
Copy link
Member

t8m commented Jul 29, 2022

There is a newline after it. image

But there is no empty line separating the commit subject and the commit message body. There should be always one empty line after the first one.

@t8m
Copy link
Member

t8m commented Jul 29, 2022

Anyway, this can be fixed when merging.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m t8m removed the approval: done This pull request has the required number of approvals label Aug 1, 2022
@t8m t8m added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Aug 1, 2022
@t8m
Copy link
Member

t8m commented Aug 1, 2022

Merged to master and 3.0 branches. Thank you for your contribution.

@t8m t8m closed this Aug 1, 2022
openssl-machine pushed a commit that referenced this pull request Aug 1, 2022
@robmcgee-mag @t8m
Updating ifdefs to account for xlclang compiler frontend on AIX.
df1e33b 
The fallback DEP works fine there. XLC should be unaffected.

CLA: trivial

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18892)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2022
@robmcgee-mag @t8m
Updating ifdefs to account for xlclang compiler frontend on AIX.
bc1f7ad 
The fallback DEP works fine there. XLC should be unaffected.

CLA: trivial

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18892)

(cherry picked from commit df1e33b)
JeffroMF pushed a commit to JeffroMF/openssl.1.55555 that referenced this pull request Aug 5, 2022
@robmcgee-mag @JeffroMF
Updating ifdefs to account for xlclang compiler frontend on AIX.
7fa4123 
The fallback DEP works fine there. XLC should be unaffected.

CLA: trivial

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#18892)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@robmcgee-mag @sftcd
Updating ifdefs to account for xlclang compiler frontend on AIX.
f8b5331 
The fallback DEP works fine there. XLC should be unaffected.

CLA: trivial

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18892)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@tmshort tmshort tmshort approved these changes

@paulidale paulidale paulidale 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 cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources 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.
5 participants
@robmcgee-mag @t8m @tmshort @paulidale @openssl-machine