Conversation
With `-Wp,-D_GLIBCXX_ASSERTIONS`, tests fail with an assertion error of the form: ``` /usr/include/c++/12/bits/stl_vector.h:1122: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _T p = float; _Alloc = std::allocator<float>; reference = float&; size_type = long unsigned int]: Assertion '__n < this->size()' failed. ``` The assertion failure happens on line 473 in report_reader.cpp: ``` std::copy(&buffer[gid_start], &buffer[gid_end], &data_ptr[offset]); ``` The problem is that `gid_end` can equal the size of `buffer`, and `&buffer[gid_end]` triggers the assertion failure in that case. Diagnosed and fixed suggested by Jerry James in the Fedora package review: https://bugzilla.redhat.com/show_bug.cgi?id=2061077#c6
matz-e
left a comment
matz-e
left a comment
There was a problem hiding this comment.
Seems sensible from my side, wondering if this will work on all compilers though.
|
I'm not sure what the corresponding flags for clang are, but I see that the CI builds completed fine so perhaps these extra definitions are simply ignored? |
src/report_reader.cpp
Outdated
| } else { // Elements report | ||
| uint64_t gid_end = range.second - min; | ||
| std::copy(&buffer[gid_start], &buffer[gid_end], &data_ptr[offset]); | ||
| std::copy(buffer.begin() + gid_start, buffer.begin() + gid_end, &data_ptr[offset]); |
There was a problem hiding this comment.
Hmmmm... Do you mind to clarify why the change, please? I don't mind, but if we do change this, we should also change the code in line 470 and we must verify that the performance of the copies remains the same (e.g., this operation is executed thousands of times in thousands of timesteps, it matters!).
On a side note, perhaps using something like std::next(buffer.begin(), gid_start) as well, but this is irrelevant.
There was a problem hiding this comment.
Turning on the _GLIBCXX_ASSERTIONS makes doing the subscript on buffer look like &buffer[buffer.size()], which would be an out of bounds error and thus undefined behavior. AFAIK, this is ok in this specific case as long as std::copy doesn't deref the pointer.
The change means we use the std::copy overload that takes iterators, and is "safer", because there is no chance there is an OOB deref, as long as gid_end never is larger than the size of the array.
The ASM will look the same from both of them, I'd expect, at a reasonable optimization level.
There was a problem hiding this comment.
Thanks @mgeplf for clarifying. My point was that accessing the array with [] in std::copy might avoid boundary checks or additional code, but if you feel that it should be the same, then no complains at all.
I'd still prefer the use of std::next(buffer.begin(), gid_start) and std::next(buffer.begin(), gid_end) with iterators, although I don't mind leaving the code as it is with the + operator.
There was a problem hiding this comment.
but if you feel that it should be the same, then no complains at all.
Had a quick look on godbolt to confirm (which I never take as conclusive), but it seems that compiler sees through this:
https://godbolt.org/z/8dez5nEM3
I'd still prefer the use of std::next(buffer.begin(), gid_start)
Ok, I'll make those changes, and another one to remove the pointer usage altogether.
There was a problem hiding this comment.
I've made the changes; any/all reviews welcome.
There was a problem hiding this comment.
All tests pass in my Fedora build with the newest changes 👍
src/report_reader.cpp
Outdated
| } else { // Elements report | ||
| uint64_t gid_end = range.second - min; | ||
| std::copy(&buffer[gid_start], &buffer[gid_end], &data_ptr[offset]); | ||
| std::copy(buffer.begin() + gid_start, buffer.begin() + gid_end, &data_ptr[offset]); |
There was a problem hiding this comment.
Turning on the _GLIBCXX_ASSERTIONS makes doing the subscript on buffer look like &buffer[buffer.size()], which would be an out of bounds error and thus undefined behavior. AFAIK, this is ok in this specific case as long as std::copy doesn't deref the pointer.
The change means we use the std::copy overload that takes iterators, and is "safer", because there is no chance there is an OOB deref, as long as gid_end never is larger than the size of the array.
The ASM will look the same from both of them, I'd expect, at a reasonable optimization level.
sergiorg-hpc
left a comment
sergiorg-hpc
left a comment
There was a problem hiding this comment.
Sorry for the delay, looks good to me 👍. Thank you @mgeplf and @sanjayankur31 for the improvements!
4 participants
Fixes an assertion error thrown when
_GLIBCXX_ASSERTIONSis defined, and enables the option by default