0

The problem: I am trying to allocate memory for an array of std::string, then using new(...)[]() to initialize it. Then I try to assign values to the array elements and this causes the application to crash (Access violation). Question is: Am I missing some compiler flag or something obvious?

Compiling this with

cl.exe /DEBUG /EHsc /MTd /Zi test.cc

produces an executable that crashes (tested in VS 2017 and VS 2012). On a side note it works as expected on Linux with GCC.

#include <iostream>
#include <string>

struct S {
  int a,b;
  S() : a(99), b(299) {}
  S & operator=(const char * rhs) { a = 100; b = 300; return *this; }  
};
std::ostream & operator<<(std::ostream & os, const S & s) { os << "(" << s.a << "," << s.b << ")"; return os; }  


typedef std::string T;
//typedef S T;
int main(int argc, char ** argv) {
    size_t N = 100;
    std::allocator<T> mem;
    T * data = mem.allocate(N);
    new(data)T[N]();

    for (ptrdiff_t i = 0; i < N; ++i)
        data[i] = "HELLO WORLD";
    for (ptrdiff_t i = 0; i < N; ++i)
        std::cout << data[i] << std::endl;
}

I tried using the other typedef, and in that case the initialization works exactly as expected.

EDIT: I see the same crash if I use C calloc for allocation instead of std::allocator.

SOLVED. It turns out that the problem is this: The C++ standard says that the new T[N] expression allocates at least sizeof(T)*N bytes. Visual C allocates an extra overhead. The placement new new(ptr)T[N] assumes that the extra overhead is available in ptr. The solution from the answer below is to loop and construct each item separately.

UlfW
  • 13
  • 3
  • 10
    Why are you using `std::allocator` and placement `new`? What is the use-case for it? What is the actual problem you try to solve with it? Why can't you use ordinary `new[]`? Why can't you use `std::vector` instead? Your question is a good example of [the XY problem](http://xyproblem.info/), where you ask for help with a solution to an (for us) unknown problem. – Some programmer dude Jun 21 '17 at 11:34
  • Isn't your `T[N]` non-standard C++? (since `N` is not a compile-time constant). Btw, also works using clang compiler under OSX – Walter Jun 21 '17 at 11:56
  • @Walter You can use variables when using `new[]`. – Some programmer dude Jun 21 '17 at 12:00
  • @Some programmer dude: The original code was tangled in thousands of lines of recursive templates. – UlfW Jun 21 '17 at 12:49
  • @UlfW don't forget to accept an answer if it sufficiently answers your question :P – PeterT Jun 23 '17 at 11:58

3 Answers3

1

The issue is with the unspecified overhead as described here very well: Array placement-new requires unspecified overhead in the buffer?

A quick fix would be

#include <iostream>
#include <string>

struct S {
  int a,b;
  S() : a(99), b(299) {}
  S & operator=(const char * rhs) { a = 100; b = 300; return *this; }  
};
std::ostream & operator<<(std::ostream & os, const S & s) { os << "(" << s.a << "," << s.b << ")"; return os; }  


typedef std::string T;
//typedef S T;
int main(int argc, char ** argv) {
    size_t N = 100;
    std::allocator<T> mem;
    T * datastor = mem.allocate(N+1);
    T * data = new(datastor)T[N]();

    for (ptrdiff_t i = 0; i < N; ++i)
        data[i] = "HELLO WORLD";
    for (ptrdiff_t i = 0; i < N; ++i)
        std::cout << data[i] << std::endl;
}

But this is not guaranteed to work always since the "unspecified overhead" might theoretically be larger than sizeof(std::string).

The version without unspecified behavior would new up the elements individually like so:

#include <iostream>
#include <string>

struct S {
  int a,b;
  S() : a(99), b(299) {}
  S & operator=(const char * rhs) { a = 100; b = 300; return *this; }  
};
std::ostream & operator<<(std::ostream & os, const S & s) { os << "(" << s.a << "," << s.b << ")"; return os; }  


typedef std::string T;
//typedef S T;
int main(int argc, char ** argv) {
    size_t N = 100;
    std::allocator<T> mem;
    T * data = mem.allocate(N);
    for (ptrdiff_t i = 0; i < N; ++i)
        new(&data[i]) T();
    for (ptrdiff_t i = 0; i < N; ++i)
        data[i] = "HELLO WORLD";
    for (ptrdiff_t i = 0; i < N; ++i)
        std::cout << data[i] << std::endl;
    for (ptrdiff_t i = 0; i < N; ++i)
        data[i].~T();
    mem.deallocate(data, N);
}
PeterT
  • 7,981
  • 1
  • 26
  • 34
0

Aren't you supposed to use construct?

#include <iostream>
#include <string>

struct S {
  int a,b;
  S() : a(99), b(299) {}
  S & operator=(const char * rhs) { a = 100; b = 300; return *this; }
};
std::ostream & operator<<(std::ostream & os, const S & s) { os << "(" << s.a << "," << s.b << ")"; return os; }


typedef std::string T;
//typedef S T;
int main(int argc, char ** argv) {
    size_t N = 100;
    std::allocator<T> mem;
    T * data = mem.allocate(N);
    //new(data)T[N]();

    for (ptrdiff_t i = 0; i < N; ++i)
        mem.construct(data + i, "HELLO WORLD");
        //data[i] = "HELLO WORLD";
    for (ptrdiff_t i = 0; i < N; ++i)
        std::cout << data[i] << std::endl;
}
  • The `construct` function will become deprecated in the C++17 standard. – Some programmer dude Jun 21 '17 at 12:04
  • Using construct instead of new gives me the same crash (i.e. initializing with the default constructor, then assign values to the strings using `operator=`). – UlfW Jun 22 '17 at 07:57
  • My bad. Using construct seems to work (both Visual C and gcc/linux). I am still not sure what is incorrect about using `new(...)T[]`. – UlfW Jun 22 '17 at 08:20
  • @UlfW see https://stackoverflow.com/questions/8720425/array-placement-new-requires-unspecified-overhead-in-the-buffer the length is prepended. So `data = new(data)T[N]();` works, if you allocate more than N elements (unspecified overhead) – PeterT Jun 22 '17 at 09:32
0

The claim that it "works" with GCC is something of an exaggeration:

g++ -std=c++17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++      16486983.cpp    -o 16486983
16486983.cpp: In member function ‘S& S::operator=(const char*)’:
16486983.cpp:7:30: warning: unused parameter ‘rhs’ [-Wunused-parameter]
   S & operator=(const char * rhs) { a = 100; b = 300; return *this; }
                              ^~~
16486983.cpp: In function ‘int main(int, char**)’:
16486983.cpp:20:10: error: ‘ptrdiff_t’ was not declared in this scope
     for (ptrdiff_t i = 0; i < N; ++i)
          ^~~~~~~~~
16486983.cpp:20:10: note: suggested alternatives:
In file included from /usr/include/c++/6/iostream:38:0,
                 from 16486983.cpp:1:
/usr/include/x86_64-linux-gnu/c++/6/bits/c++config.h:202:28: note:   ‘std::ptrdiff_t’
   typedef __PTRDIFF_TYPE__ ptrdiff_t;
                            ^~~~~~~~~
/usr/include/x86_64-linux-gnu/c++/6/bits/c++config.h:202:28: note:   ‘std::ptrdiff_t’
16486983.cpp:20:27: error: ‘i’ was not declared in this scope
     for (ptrdiff_t i = 0; i < N; ++i)
                           ^
16486983.cpp:22:10: error: ‘ptrdiff_t’ was not declared in this scope
     for (ptrdiff_t i = 0; i < N; ++i)
          ^~~~~~~~~
16486983.cpp:22:10: note: suggested alternatives:
In file included from /usr/include/c++/6/iostream:38:0,
                 from 16486983.cpp:1:
/usr/include/x86_64-linux-gnu/c++/6/bits/c++config.h:202:28: note:   ‘std::ptrdiff_t’
   typedef __PTRDIFF_TYPE__ ptrdiff_t;
                            ^~~~~~~~~
/usr/include/x86_64-linux-gnu/c++/6/bits/c++config.h:202:28: note:   ‘std::ptrdiff_t’
16486983.cpp:22:27: error: ‘i’ was not declared in this scope
     for (ptrdiff_t i = 0; i < N; ++i)
                           ^
16486983.cpp:14:14: warning: unused parameter ‘argc’ [-Wunused-parameter]
 int main(int argc, char ** argv) {
              ^~~~
16486983.cpp:14:28: warning: unused parameter ‘argv’ [-Wunused-parameter]
 int main(int argc, char ** argv) {
                            ^~~~

These need to be fixed; thankfully that's not hard:

#include <cstddef>
#include <iostream>
#include <string>

struct S {
  int a,b;
  S() : a(99), b(299) {}
  S & operator=(const char*) { a = 100; b = 300; return *this; }
};
std::ostream & operator<<(std::ostream & os, const S & s) { os << "(" << s.a << "," << s.b << ")"; return os; }


//typedef std::string T;
typedef S T;
int main() {
    std::size_t N = 100;
    std::allocator<T> mem;
    T * data = mem.allocate(N);
    new(data)T[N]();

    for (std::size_t i = 0; i < N; ++i)
        data[i] = "HELLO WORLD";
    for (std::size_t i = 0; i < N; ++i)
        std::cout << data[i] << std::endl;
}

Now it runs, almost¹ Valgrind-clean. But the naked placement-new looks strange - if you're using an allocator, you should use its construct():

for (std::size_t i = 0; i < N; ++i)
    mem.construct(data+i);

¹Don't forget to release the memory:

for (std::size_t i = 0; i < N; ++i)
    mem.destroy(data+i);
mem.deallocate(data, N);
Community
  • 1
  • 1
Toby Speight
  • 27,591
  • 48
  • 66
  • 103
  • Hmm, this seems to be more of a code review than an answer. But I hope it provides some insight, anyway. What does Valgrind say on the failing platforms? That should help diagnose any memory errors. – Toby Speight Jun 21 '17 at 14:11