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

[C API] GetOrdering/IsAtomicSingleThread do not work for fence/load/store #65227

Closed
Benjins opened this issue Sep 3, 2023 · 1 comment · Fixed by #65228
Closed

[C API] GetOrdering/IsAtomicSingleThread do not work for fence/load/store #65227

Benjins opened this issue Sep 3, 2023 · 1 comment · Fixed by #65228

Comments

@Benjins
Copy link
Contributor

Benjins commented Sep 3, 2023

Two related issues with atomics in the C API:

  • LLVMGetOrdering currently does not support fence instructions
  • LLVMIsAtomicSingleThread currently does not support fence or load/store instructions

In both cases, we will cast the unsupported instruction types to an unrelated type (whatever the else branch assumes it is), which will result in reading garbage. This will crash if assertions are enabled, otherwise it will just return whatever value it gets: in the case of LLVMGetOrdering this can return invalid enum values

@Benjins
Copy link
Contributor Author

Benjins commented Sep 3, 2023

I put together a PR for a patch that should fix this: if that should still be on Phabricator I can re-post it there

#65228

nikic pushed a commit that referenced this issue Sep 30, 2023
… instrs (#65228)

Fixes #65227

LLVMGetOrdering previously did not support Fence instructions, and
calling it on a fence would lead to a bad cast as it
assumed a load/store, or an AtomicRMWInst. This would either read a
garbage memory order, or assertion

LLVMIsAtomicSingleThread did not support either Fence instructions,
loads, or stores, and would similarly lead to a bad cast.
It happened to work out since the relevant types all have their synch
scope ID at the same offset, but it still should be fixed

These cases are now fixed for the C API, and tests for these
instructions are added. The echo test utility now also supports cloning
Fence instructions, which it did not previously

-----

From what I can tell, there's no unified API to pull
`getOrdering`/`getSyncScopeID` from, and instead requires casting to
individual types: if there is a better way of handling this I can switch
to that
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants