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.

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

Refine atomic operation flags in bh_atomic #2780

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

no1wudi
Copy link
Collaborator

@no1wudi no1wudi commented Nov 16, 2023

This also fix the build break in spec test of NuttX.

@no1wudi no1wudi force-pushed the main branch 2 times, most recently from f81f02b to e0aed13 Compare November 16, 2023 09:00
@no1wudi no1wudi changed the title Refine atoimic operation flags in bh_atomic Refine atomic operation flags in bh_atomic Nov 16, 2023
core/shared/utils/bh_atomic.h Outdated Show resolved Hide resolved
core/shared/utils/bh_atomic.h Outdated Show resolved Hide resolved
core/shared/utils/bh_atomic.h Show resolved Hide resolved
@no1wudi no1wudi force-pushed the main branch 4 times, most recently from 91e942c to 25864e8 Compare November 16, 2023 10:02
@no1wudi no1wudi requested a review from wenyongh November 16, 2023 10:32
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@yamt
Copy link
Collaborator

yamt commented Nov 16, 2023

i have a concern in the original change which introduced atomic 16.
#2682 (comment)

* reference to `__atomic_fetch_add_2'
*/
#ifndef WASM_UINT16_IS_ATOMIC
#if !defined(__linux__) && defined(__riscv)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you mean by !defined(__linux__) here?
do you mean defined(__NuttX__)?

also, why do you override it here, rather than having it in platform-specific headers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess all non-linux/baremetal platforms has the same issue, not only nuttx, since the usage of the baremetal toolchain have nothing difference with other RTOS platform.

And you can also define the behavior in platfrom-specific setting, explicit settings always have the highest priority.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about freebsd?

Copy link
Collaborator Author

@no1wudi no1wudi Nov 17, 2023

Choose a reason for hiding this comment

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

I guess the bsd family should be OK as linux, but I don't have a test on it.

Copy link
Collaborator

@yamt yamt left a comment

Choose a reason for hiding this comment

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

lgtm

@wenyongh wenyongh merged commit be05af7 into bytecodealliance:main Nov 18, 2023
383 checks passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
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