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

Fix translation of atomic functions #685

Merged
merged 6 commits into from
Oct 11, 2024
Merged

Fix translation of atomic functions #685

merged 6 commits into from
Oct 11, 2024

Conversation

schuessf
Copy link
Contributor

@schuessf schuessf commented Oct 8, 2024

PR #657 added support for atomic functions. This had however two problems (in the current version):

  • To improve our performance, this PR tried to avoid unecessary heap variables, i.e., we tried to translate __atomic_store_n(&x, 1, 5) to x := 1 inside an atomic block rather than write~(x, 1) (see d08b25f and in the PR). This way the arguments were dispatched possibly multiple times, which is problematic if they have side-effects (see atomic_load_n_sideeffects.c).
  • We put everything inside an atomic-block. However, not everything is guaranteed to be executed atomically, e.g. for __atomic_store_n(&x, expr1, expr2) only the actual write on x is atomic, not the evaluation of expr1 or expr2. This could have led to missed data-races (see atomic_memory_order.c)
  • We considered the read and write to be atomic for __atomic_load, but only the read should be atomic (fixed in f2e474d)

To fix these issues, the PR adds an interface IPointerReadWrite (and two implementations) that allows to get the dispatched argument, a read and a write separately (s.t. we can "control" the atomicity) and that guarantees that the dispatch is only done once. This interface is the used in StandardFunctionHandler for the translation of atomic functions.

To fix these issues, the PR adds three separate method to ExpressionResultTransformer:

  • dispatchPointerLValue to dispatch the pointer expression, but tries to produce LocalLValue
  • makePointerAssignment writes to the given address, produces simple assignment for LocalLValue and write-call for HeapLValue
  • readPointerValue reads from the given address, always returns an aux-var as RValue, assigns the LocalLValue to the aux-var or reads to the aux-var from the memory for HeapLValue

Previously we translated C-expressions possibly twice due to the optimization for pointer reads/writes and made everything atomic (to handle stdatomic functions).
Now, we make sure that the expression is only translated once and we separate the ExpressionResult of the original expressions and the corresponding read/write, s.t. we can control the atomicity for stdatomic functions properly.
Therefore we replaced the methods by an interface IPointerReadWrite (and matching implementations).
Copy link
Contributor

@maul-esel maul-esel left a comment

Choose a reason for hiding this comment

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

Thanks for making sure these functions work correctly!

I took a look, and it seems a good change. I was not able to check every atomic function in precise detail though. I left some comments on the general mechanism, mostly asking for documentation / explanation.

@schuessf schuessf requested a review from maul-esel October 10, 2024 10:09
@schuessf
Copy link
Contributor Author

Also, just for my understanding: Could you explain how the two classes implementing this interface differ from HeapLValue vs LocalLValue ? Do we need both distinctions?

@maul-esel This interface was not really necessary, HeapLValue and LocalLValue are sufficient for distinction.
Therefore I replaced this interface with separate methods and also added some documentation.
So please review this PR again.

@schuessf schuessf merged commit 4a9d9a5 into dev Oct 11, 2024
2 of 4 checks passed
@schuessf schuessf deleted the wip/fs/atomic-fix branch October 11, 2024 09:13
schuessf added a commit that referenced this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants