Probably problem with SSO for std::string. #36
Comments
|
Hi, just wanted to let you know I've reproduced this and I'll have a look at it. Thanks for the report. |
|
Is there any progress on this? I noticed that increasing @mattiasflodin Are you sure using relaxed atomics is ok here? https://github.com/mattiasflodin/reckless/blob/v3.0.2/reckless/include/reckless/detail/platform.hpp#L76 |
|
Maybe this demo will be useful https://github.com/mopleen/reckless_demo_crash Also, logging only long strings does not crash either. Short string is what's triggering the problem. |
|
@serpent7776 @mopleen Thank you for your patience with this bug and your efforts to provide reproduction demos and inspecting the code. I have been busy with some other projects so this had to be put on hold for a while. Anyway, I believe I have figured out what is going wrong. The usual suspect would of course be multithreading and incorrect use of atomics, but in fact this error is not related to that. The problem lies in the ring buffer, which is implemented by mapping multiple virtual addresses to the same physical memory in an attempt to avoid some wraparound checks. _M_dispose()
{
if (!_M_is_local())
_M_destroy(_M_allocated_capacity);
}
For some reason that I'm currently looking into, the same virtual address is not being used when the object is destroyed as when it is created (even though they refer to the same physical memory), which causes It may turn out that my ring buffer is too clever for its own good and that I need to change its implementation. I hope not, because that could make the performance of pushing very large objects unintuitive. But so far I believe there is just a bug in some pointer arithmetic related to the buffer. |
basic_log::output_worker calculates frame positions by taking the "front" address (current read pointer) from mpsc_ring_buffer and then advancing that until it reaches the end of the batch. However, if the batch size is large enough to go past the end of the ring buffer then no wrap around is applied to the frame pointer. This isn't strictly a buffer overrun because, due to the nature of the ring buffer implementation, the same physical memory is mapped in a second time at the end and we are in fact meant to go past the end in situations where objects overlap the end. However, we can run into trouble when an object was constructed in the first virtual memory-map and then accessed through the second virtual-memory map. For plain old data it works fine, but if the object has code that depends on the object's this-pointer staying consistent (a reasonable expectation) then formatting or destroying the object can lead to undefined behavior. This surfaced as github issue #36 in which the libstdc++ implementation of std::string had optimizations that would make use of the address of a class member to determine which deallocation strategy must be used. The fix is fairly simple: instead of using the frame pointer as loop invariant we use the ring buffer's 64-bit position to determine when the end is reached, and whenever we need to use the frame pointer we make sure that it is consistently wrapped to always start in the first virtual-memory block.
basic_log::output_worker calculates frame positions by taking the "front" address (current read pointer) from mpsc_ring_buffer and then advancing that until it reaches the end of the batch. However, if the batch size is large enough to go past the end of the ring buffer then no wrap around is applied to the frame pointer. This isn't strictly a buffer overrun because, due to the nature of the ring buffer implementation, the same physical memory is mapped in a second time at the end and we are in fact meant to go past the end in situations where objects overlap the end. However, we can run into trouble when an object was constructed in the first virtual memory-map and then accessed through the second virtual-memory map. For plain old data it works fine, but if the object has code that depends on the object's this-pointer staying consistent (a reasonable expectation) then formatting or destroying the object can lead to undefined behavior. This surfaced as github issue #36 in which the libstdc++ implementation of std::string had optimizations that would make use of the address of a class member to determine which deallocation strategy must be used. The fix is fairly simple: instead of using the frame pointer as loop invariant we use the ring buffer's 64-bit position to determine when the end is reached, and whenever we need to use the frame pointer we make sure that it is consistently wrapped to always start in the first virtual-memory block.
|
Resolved in v3.0.3. |
|
Seems it's working fine now, thanks. |
Code:
Result in:
Randomly I saw also:
If
strhas more than 15 characters problem doesn't appear. @mattiasflodin Could you take a look at that? Probably it is similar to #35 (also short string).The text was updated successfully, but these errors were encountered: