Skip to content

CDRIVER-4215 use legacy atomic built-ins on gcc (>= 4.1, < 4.9) #889

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

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

rcsanchez97
Copy link
Contributor

This work is not yet complete. However, given the scope of the change just in getting the compile to succeed on RHEL 6, it seemed prudent to push my first set of changes as a WIP PR.

Initial patch build: https://spruce.mongodb.com/version/61874ce932f4177a1ad61efb/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

@vector-of-bool In the patch build, you will want to focus on the compile task in the rhel62 variant. The test tasks are still failing, but for unrelated reasons. At this point I am specifically interested to know if you find any errors or deficiencies in my initial changes.

After I get the compile working on RHEL 7 I will take a look at all the warnings that are still being produced.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

The changes LGTM. The atomic test cases are thorough enough that they will recognize any obviously-wrong changes.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Changes look good. Thank you for investigating. I have comments about macros since they are in public headers, but no concerns about correctness.

@rcsanchez97 rcsanchez97 changed the title WIP: CDRIVER-4215 use legacy atomic built-ins on gcc (>= 4.1, < 4.7) CDRIVER-4215 use legacy atomic built-ins on gcc (>= 4.1, < 4.9) Nov 11, 2021
@rcsanchez97
Copy link
Contributor Author

@rcsanchez97
Copy link
Contributor Author

The last changes that I pushed constitute two things:

  • a tweak to .evergreen/run-tests.sh to ensure that registering the CA certificate succeeds
  • the atomics changes are now confined to bson-atomics.h

The full patch build that I submitted shows that on RHEL 6 and 7 the compile tasks and now some of the tests pass. There are still numerous test tasks that are failing, but those failures are unrelated.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM

Assert (got, ==, 9); \
Assert (value, ==, 9); \
/* Compare-exchange-weak succeed: */ \
/* 'weak' may fail spuriously, so it must *eventually* succeed */ \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't argue with that haha.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

LGTM, but one comment about naming and clarity on a macro

@rcsanchez97
Copy link
Contributor Author

@vector-of-bool Does that comment work for you?

@vector-of-bool
Copy link
Contributor

Yes, that comment is helpful

@rcsanchez97 rcsanchez97 merged commit cdfb5e0 into mongodb:master Nov 11, 2021
@rcsanchez97 rcsanchez97 deleted the cdriver-4215 branch November 11, 2021 20:38
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.

3 participants