Bug in a for-loop-copy vs std::copy which I don't understand-CodePudding

Below is a very simple (stripped from most of the template code) fifo buffer for a serial output interface. I decided to write my own because a) I'm trying to learn C , b) the container adapter queue doesn't bring with it the "quickly-put-many" facility, enforcing an explicit push() for every element. I know that many modern compilers in many circumstances may optimize this, but for the sake of a) I wanted to do this myself - feel free to comment on the idea and any style/methodic errors you deem noteworthy.

The question however just deals with the inner loop of the "quickly-put-many" function put(). Compiled with the std::copy() variant, everything looks ok, but with my own version of the insertion (-DBUGGY), the data is partially clobbered.

#include <cstddef>
#include <array>
#include <vector>
#include <atomic>
#include <algorithm>
#include <type_traits>
#include <iterator>
#include <iostream>
#include <string>
#include <queue>
#include <chrono>



struct SerialBuffer
{
  std::array<char,127> fifo{};
  std::atomic<int8_t> hd = 0, tl = 0, vtl = 0;

  int8_t space(void) // return free space in queue
  {
    volatile int8_t tmp = hd - vtl - 1;
    if (tmp < 0) { tmp  = 127; }
    return tmp;
  }

  int8_t reserve(int8_t n) // move virtual tail at once, reserving a run of bytes at end
  {
    volatile int8_t new_vtl = vtl;
    if (n <= space()) {
      if (new_vtl - 127   n >= 0) { vtl = new_vtl - 127   n; }
      else { vtl = new_vtl   n; }
      return new_vtl;
    }
    return -1;
  }

  int8_t next(int8_t i) // advance index in queue
  {
    if (i >= 127 - 1) { return 0; }
    return i   1;
  }

  void confirm(void) // confirm the formerly reserved bytes as present in queue
  {
    tl = static_cast<int8_t>(vtl);
  }
  
  int8_t headroom(int8_t i) // return number bytes from queue index to queue end
  {
    return 127 - i;
  }
  
  template<typename iter_t>
  bool put(iter_t it, int8_t n) // (source, number of bytes)
  {
    int8_t i = reserve(n);

    if (i >= 0) {
      int8_t j = std::min(n, headroom(i)); // maybe two consecutive insert-ranges: first from i to buffer end, rest from buffer start

#ifdef BUGGY
      for (; i < 127; i  ) {
        fifo[i] = *it  ;
      }
      for (i = 0; i < n-j; i  ) {
        fifo[i] = *it  ;
      }
#else
      std::copy(it, it j, fifo.begin() i);
      std::copy(it j, it n, fifo.begin());
#endif
      
      confirm(); 
      return true;
    }
    return false;
  }
    
  bool put(std::vector<char> v) { return put(v.cbegin(),v.size()); }
  bool put(std::basic_string<char> v) { return put(v.cbegin(),v.size()); }

  
  void dump(int8_t k = 127)
  {
    if (space() < k) { hd = static_cast<int8_t>(tl); }
    else { hd = (hd   k) % 127; }
  }
  
  void print(void)
  {
    std::cout << "Head:" << (0 hd) << " Tail:" << (0 tl) << " VirtTail:" << (0 vtl) << std::endl;
    for (int8_t i = hd; i != tl; i = next(i)) { std::cout << fifo[i]; }
    std::cout << std::endl;
  }
};


int main(void)
{
  using namespace std::string_literals;
  
  SerialBuffer fifo1;
  auto tmp{"/uwb/x1/raw:123456789"s};
  std::vector<char> x(tmp.cbegin(),tmp.cend());

  std::queue<char,std::array<char,127>> fifo2;

  for (auto _: {1,2,3}) {
    for (int i=0; i < 10'000'000; i  ) {
      if (!fifo1.put(x)) fifo1.dump();
    }
    fifo1.print();   
  }
}

Results:

$ g   bug.cpp --std=c  17 -O3 && ./a.exe
Head:52 Tail:115 VirtTail:115
/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789
Head:104 Tail:103 VirtTail:103
/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789
Head:28 Tail:70 VirtTail:70
/uwb/x1/raw:123456789/uwb/x1/raw:123456789

$ g   bug.cpp --std=c  17 -O3 -DBUGGY && ./a.exe
Head:52 Tail:115 VirtTail:115
/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789
Head:104 Tail:103 VirtTail:103
▒ե▒qс▒▒1▒3▒▒wb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789/uwb/x1/raw:123456789
Head:28 Tail:70 VirtTail:70
/uwb/x1/raw:123456789/uwb/x1/raw:123456789

As you can see, there are garbled bytes in the second run. I am puzzled where my error in those seemingly harmless for loops is.

EDIT: As @yzt pointed out, this was an embarrasing simple logic error. I wrote a (correct) first for based version, then changed to std::copy then, too late in the evening, tried to measure the runtime difference by rewriting the for loops, this time wrong. Sorry all, this was a derivative of the "don't commit and go home when it doesn't run" error. Correct code:

  n -= j;
  for (; j > 0; j--,i  ) {
    fifo[i] = *it  ;
  }
  for (i = 0; i < n; i  ) {
    fifo[i] = *it  ;
  }

CodePudding user response：

One bug is that in your first loop, when you start from 0 (which happens even at first invocation,) you'd be incrementing the vector iterator 127 times, which clearly is out of range.

The copies work and your loops don't because your loops and the std::copy calls you are making are doing different things. They don't start or stop at the same indices, and therefore they aren't copying the same stuff.

Loops similar to the copies would be:

for (ptrdiff_t index = 0; index < j;   index)
    *(fifo.begin()   i   index) = *(it   index); // the exact way you dereference isn't important; could have used array subscripting
for (ptrdiff_t index = 0; index < n - j;   index)
    *(fifo.begin()   index) = *(it   j   index);

Also, I'd suggest you use a larger type than int8_t (e.g int) for your internal, intermediate calculations. There might be some overflows or miscalculations lurking in there.