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

volatile declaration #17

Open
sslupsky opened this issue Sep 27, 2019 · 4 comments
Open

volatile declaration #17

sslupsky opened this issue Sep 27, 2019 · 4 comments

Comments

@sslupsky
Copy link

I think I may have found one issue here:

size_t _head;
size_t _numElements;

Do these variables need to be declared "volatile"? When I compiled a sketch without the volatile declaration, the isEmpty() member function always came back false.

Does this suggest _buf should be declared volatile as well?

@sslupsky
Copy link
Author

This is in the dev branch.

@wizard97
Copy link
Owner

This is a good point. Some aggressive compilers might optimize and cause this behavior. There are a few solutions:

  1. Declare the _head and `_numElements' as volatile.
  2. Declare the whole buffer as volatile.

I'm not sure about needing to declare the underlying buffer as volatile... I'm not sure if compilers are allowed to optimize entire arrays in registers. I doubt they are because most ISAs do not have an efficient instruction that supports indexing into the register file based on a dynamic offset.

Feel free to make a pull request to declare those members as volatile and I will accept it.

@sslupsky
Copy link
Author

sslupsky commented Oct 3, 2019

Will do.

@wizard97
Copy link
Owner

I know this issue is old, but commenting anyways. Volatile is deprecated in modern C++ for almost every use case (including this one). The correct solution would be inserting a compiler memory barrier upon entry and exit of the atomic guard. This will force the compiler to synchronize the backing memory corresponding to the registers contents.

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

No branches or pull requests

2 participants