Skip to content

[2.0.x] fix possible race condition#11923

Merged
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
GMagician:2.0.x-revert-parts-of-endstop-bugfix
Sep 25, 2018
Merged

[2.0.x] fix possible race condition#11923
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
GMagician:2.0.x-revert-parts-of-endstop-bugfix

Conversation

@GMagician
Copy link
Contributor

Fix some commits done in #11900

@thinkyhead
Copy link
Member

What makes you think this fixes a race condition?

@GMagician
Copy link
Contributor Author

If reset variable order is not correct you may reset first live but in meantime a temp isr may be executed and live may be updated (count is 1) and after interrupt you reset count (already 0 because of interrupt).
In this case if you are unlucky livew is not reset at all

@thinkyhead
Copy link
Member

Are you sure that the order is reversed by the existing syntax? I would expect them to be set to zero in the given order, first endstop_poll_count and then validated_live_state. Does C++ formally define this?

@thinkyhead
Copy link
Member

Apparently it does. I'd've expected the standard to be less formal and simply make it equivalent, but in strict mode the compiler will be more literal about it.

@thinkyhead thinkyhead merged commit dc11131 into MarlinFirmware:bugfix-2.0.x Sep 25, 2018
@GMagician
Copy link
Contributor Author

Not sure but I think is compiler dependent (depending on optimizations)

@GMagician
Copy link
Contributor Author

Are you sure that the order is reversed by the existing syntax? I would expect them to be set to zero in the given order, first endstop_poll_count and then validated_live_state. Does C++ formally define this?

From my memory with x86 C I remember that order is from right to left but not sure if optimizations don't change (don't know if this is strictly ruled)

@GMagician
Copy link
Contributor Author

@GMagician GMagician deleted the 2.0.x-revert-parts-of-endstop-bugfix branch September 25, 2018 20:35
@thinkyhead
Copy link
Member

Yep, that's the same post I found before posting "Apparently it does…"

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.

2 participants

Comments