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

<barrier>: Some _STL_VERIFYs are not working well #3748

Closed
achabense opened this issue Jun 6, 2023 · 4 comments · Fixed by #3757
Closed

<barrier>: Some _STL_VERIFYs are not working well #3748

achabense opened this issue Jun 6, 2023 · 4 comments · Fixed by #3757
Labels
enhancement Something can be improved fixed Something works now, yay!

Comments

@achabense
Copy link
Contributor

achabense commented Jun 6, 2023

The following piece of code will not trigger any diagnosis.

#include<barrier>

int main() {
	std::barrier bar{PTRDIFF_MIN};//0b1000...0 << 1 ~> 0

	std::barrier bar2{100};
	(void) bar2.arrive(PTRDIFF_MIN + 1);//0b1000....1 << 1 ~> 2
}

A fixed check for the ctor would be _STL_VERIFY(_Expected >= 0 and _Expected <= max(), ...);. Also please considering adding a static_assert to guarantee that max() << _Barrier_value_shift will keep positive and not lose any bit.

STL/stl/inc/barrier

Lines 81 to 86 in c8d1efb

constexpr explicit barrier(
const ptrdiff_t _Expected, _Completion_function _Fn = _Completion_function()) noexcept /* strengthened */
: _Val(_One_then_variadic_args_t{}, _STD move(_Fn), _Expected << _Barrier_value_shift) {
_STL_VERIFY(_Val._Myval2._Current.load(memory_order_relaxed) >= 0,
"Precondition: expected >= 0 and expected <= max() (N4950 [thread.barrier.class]/9)");
}

A fixed check for arrive would be _STL_VERIFY(_Update > 0 && _Update <= max(), ...), before shifting. (The second half of check is to avoid large positive values like PTRDIFF_MAX - 1.)

STL/stl/inc/barrier

Lines 95 to 98 in c8d1efb

_NODISCARD_BARRIER_TOKEN arrival_token arrive(ptrdiff_t _Update = 1) noexcept /* strengthened */ {
// Shifting before precondition check, so that exceeding max() will trigger precondition check too
_Update <<= _Barrier_value_shift;
_STL_VERIFY(_Update > 0, "Precondition: update > 0 (N4950 [thread.barrier.class]/12)");

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Jun 6, 2023

Checking of _Expected was weakened in 84f5ac6 which said "barrier micro optimizations". Was the weakening intentional?

Also, why don't we simply spell the value of _Barrier_max as PTRDIFF_MAX / 2 or PTRDIFF_MAX >> 1?

inline constexpr ptrdiff_t _Barrier_max = (1ULL << (sizeof(ptrdiff_t) * CHAR_BIT - 2)) - 1;

@AlexGuteniev

@AlexGuteniev
Copy link
Contributor

Was the weakening intentional?

Yes, I wanted to have fewer comparisons here.
I only wanted to catch negatives, positive overflow is being caught in wait loop:

STL/stl/inc/barrier

Lines 123 to 124 in c8d1efb

_STL_VERIFY(_Current >= 0, "Invariant counter >= 0, possibly caused by preconditions violation "
"(N4950 [thread.barrier.class]/12)");

Also, why don't we simply spell the value of _Barrier_max as PTRDIFF_MAX / 2 or PTRDIFF_MAX >> 1?

I've elaborated this formula to avoid including <limits> for numeric_limits, but haven't figured out it could be simpler.

@achabense
Copy link
Contributor Author

achabense commented Jun 6, 2023

For the constructor, as the check is performed on _Expected << _Barrier_value_shift, any negative _Expected with 30th/62th bit being 0 will pass the check. This error is not catchable after construction, as _Val._Myval2._Current itself has become positive.

@AlexGuteniev
Copy link
Contributor

Hm, agree

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jun 6, 2023
@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants