Array changes when element retrieved

Question

Edit: Added constructors and add function

Consider the following three blocks:

//ONE
template<typename T>
class List1 {
private:
    uint32_t capacity;
    uint32_t used;
    T* data;
    void checkGrow() {
            if (used < capacity)
                    return;
            T* old = data;
            capacity*=2;
            data = (T*)new char[sizeof(T) * capacity];
            memcpy(data, old, sizeof(T)*used);
            delete (void*)old;
    }
public:
    List1() : capacity(1), used(0), data((T*)new char [sizeof(T)]) {}
    List1(uint32_t initialSize) :
            capacity(initialSize), used(0), data((T*)new char[sizeof(T)]) {}

    List1(const List1& orig) :
            capacity(orig.capacity), used(orig.used), data((T*)new char[used * sizeof(T)]) {
            memcpy(data, orig.data, used * sizeof(T));
    }
    uint32_t serializeSize() const { return sizeof(used) + used*sizeof(T); }
    char* read(char* p) {
            used = *(uint32_t*)p;
            p += sizeof(uint32_t);
            data = (T*)new char[used*sizeof(T)];
            memcpy(p, data, used * sizeof(T));
            return p + used*sizeof(T);
    }
    char* write(char* p) {
            *(uint32_t*)p = used;
            p += sizeof(uint32_t);
            memcpy(p, data, used * sizeof(T));
            return p + used * sizeof(T);
    }

    ~List1() { delete [] data; }

    void add(const T& v) {
            checkGrow();
      data[used++] = v;        
    }
    uint32_t getUsed() const{
            return used;
    }
    uint32_t getCapacity() const{
            return capacity;
    }

    //const T& operator [](int i) const { return data[i]; }
    //T& operator [](int i) { return data[i]; }
    T getData (int i) const{
            return data[i];
    }
    uint32_t size() const { return used * sizeof(T); }
};

//TWO
List1<uint32_t> temp=in.readList<uint32_t>(); // <List1<uint32_t>>();
//BREAKPOINT HERE
for(uint i=0;i<15;i++){
    //cout<<temp[i]<<endl;
    cout<<temp.getData(i)<<endl;
}

//THREE
template<typename T>
T _read() {
    T temp = *(T*)p;
    p += sizeof(T);
    availRead -= sizeof(T);
    return temp;
}

template<typename T>
T read(){
    //cout << (uint64_t)p << endl;
    checkAvailableRead(sizeof(T));
    return _read<T>();
}
template<typename T>

List1<T> readList(){
    uint32_t len = read<uint32_t>();
    List1<T> temp(len);
    for (uint i = 0 ; i < len; i++){
        T val =read<T>();
        temp.add(val);
        //HERE: For some reason code does not work without this print statement
        //cout<<temp.getData(i)<<endl;
        }
    return temp;
}

Basically the issue is that the value of data changes from after returning from getData as shown below.

gdb) p/u *temp.data@15  
$1 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}
(gdb) p/u *temp.data@15  
$2 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}
(gdb) p/u *data@15  
$3 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}
[New Thread 8444.0xad8]
(gdb) p/u *data@15  
$4 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}
[New Thread 8444.0x214c]
(gdb) p/u *temp.data@15  
$5 = {0, 1, 2, 3, 4, 5, 83, 0, 2150008464, 1, 3742232646, 0, 168272, 6, 0}

For some reason adding a print statement to readList fixes the issue, but that isn't a reasonable solution. I've tried a few different variations on the code but none of them worked.

I'm not sure what the issue is or how to even begin debugging as the problem occurs between the return statement and the next iteration of the loop (there's nothing to step into there).

Any advice would be greatly appreciated.

Probably rule of 3 violation. Need to see your constructors (& destructor). — Mat, Dec 17 '18 at 06:24
A read access like in `return data[i];` with `T` as return (a value) shouldn't change any contents of anything else except there is undefined behavior. This could be a sign that your list storage and the return value share storage. That must not be - it's a bug. I would check for proper allocation... (Yeah, or something else wrong like mentioned by @Mat.) — Scheff's Cat, Dec 17 '18 at 06:26
_For some reason adding a print statement to readList fixes the issue_ another thing how [Undefined Behavior](https://stackoverflow.com/a/4105123/1505939) may expose... — Scheff's Cat, Dec 17 '18 at 06:29
How _add()_ is implemented ? if _data_ point to a too small vector or a wrong address _getData()_ can read memory (including stack) modified elsewhere and may be through your printing. I remark that the values into the vector are equals to their indexes which is also the parameter of _getData()_ called by the printing. So as it was already said give us the definition of your constructor(s) and _add()_ — bruno, Dec 17 '18 at 07:24
One of your three constructors allocates enough space to hold the indicated capacity. Another allocates enough space to hold the amount used. The third allocates space that matches neither the amount used nor the capacity. You should probably pick one uniform way of doing things. — Mark Plotnick, Dec 19 '18 at 03:32

David Schwartz · Answer 1 · 2018-12-18T06:05:13.907

1

List1(const List1& orig) :
        capacity(orig.capacity), used(orig.used), data((T*)new char[used * sizeof(T)]) {
        memcpy(data, orig.data, used * sizeof(T));
}

For List1 to work correctly, there must never be a List1 whose capacity is greater than the actual allocated size. However, this creates a new List1 that violates this invariant if orig has a capacity greater than its used.

You probably meant capacity(orig.used).

Same issue here:

List1(uint32_t initialSize) :
        capacity(initialSize), used(0), data((T*)new char[sizeof(T)]) {}

If you set capacity to initialSize, you can't allocate space for just 1 T.

This is also broken delete (void*)old;. What you allocate with new[], you must free with delete[].

Note that List1 can only be used to hold POD types (plain old data) that don't have constructors or destructors. If you're trying to use List1 to hold anything more complex, you're way off base with your design.

edited Dec 18 '18 at 06:05

answered Dec 17 '18 at 20:43

David Schwartz

179,497
17
214
278

I'll give that a try, but for the context that constructor is used in it shouldn't matter. Will post afterwards. – tpnam Dec 17 '18 at 20:48
@tpnam You didn't give us the code to reproduce the error. – David Schwartz Dec 17 '18 at 20:50
Above didn't work. I'll see if I can put together a working example within a single file as the various components above are all separate. – tpnam Dec 17 '18 at 21:03
This is also broken `delete (void*)old;`. What you allocate with `new[]`, you must free with `delete[]`. – David Schwartz Dec 17 '18 at 21:06

Array changes when element retrieved

1 Answers1