2

I am doing a project with C++ and SFML, compiled with gcc version 6.3.0 (MinGW.org GCC-6.3.0-1), a dungeon crawler game. I am trying to create a layout by a bidimensional vector of Rooms (a class I also made, header is Room::Room(std::string layout)), and the layouts are created via a seed that stablishes the characteristics of any room, this is separated room-by-room with a slash symbol (/)

I start by reading the seed until it finds the slash, then it creates an instance of Room and adds it into the vector

/* Get Level Layout */
layout = getLayout(level); //Calls SQLite DB
layout_dim = getLayoutDim(level); //Calls SQLite DB
rows = layout_dim[0];
cols = layout_dim[1];

std::vector<std::vector<Room*>> layout_2d(rows);

std::string acumul = "";

int current_row = 0;
int current_col = 0;

for (size_t i = 0; i < strlen(layout.c_str()); i++){
    if(layout[i] == '/'){
        Room* r = new Room(acumul);
        layout_2d[current_row][current_col] = r;

        if(current_col + 1 == cols){ //That was last column on the row
            current_row ++;
            current_col = 0;
        }
        else{
            current_col ++;
        }

        acumul = "";
    }
    else{
        acumul = acumul + layout[i];
    }
}

When it starts to create the instance, (Room* r = new Room(acumul);) everything goes as expected, the "seed" for the room "0 1001 0 0" is OK

npos:4294967295
_M_dataplus
    std::allocator<char> (base):
    std::allocator<char>
_M_p:
    0x8bdd98 "0 1001 0 0"
_M_string_length: 10

But when it enters the constructor of class Room, changes into this

npos: 4294967295
_M_dataplus
    std::allocator<char> (base):
    std::allocator<char>
_M_p:
    0x8bddd0 "�݋"
_M_string_length: 9166224

Looking at the Hex Editor that comes with VS Code, looks like the pointer has moved: Here it is when goes OK

And here when goes NOK

Ask me if you need more info, and thanks in advance

EDIT: I have the following trace when I launch with properties -fsanitize=address,undefined -Wall -Wextra

g++ main.cpp sMenu.cpp sSettings.cpp sGame.cpp Room.cpp ConfigFile.cpp common.cpp db.cpp sqlite3.o -fdiagnostics-color=always -fsanitize=address,undefined -Wall -Wextra -o C:\Users\ricc_\Documents\Proyectos\Development\madou4cplusplus\src\..\dist\madou4.exe -L..\lib -I..\include -lsfml-graphics -lsfml-audio -lsfml-window -lsfml-system -g
sGame.cpp: In member function 'int sGame::getDataFromSeed(int)':
sGame.cpp:32:32: warning: unused parameter 'seed' [-Wunused-parameter]
 int sGame::getDataFromSeed(int seed){
                                ^~~~
sGame.cpp: In member function 'virtual std::__cxx11::string sGame::Run(sf::RenderWindow&)':
sGame.cpp:72:19: warning: unused variable 'r' [-Wunused-variable]
             Room* r = new Room(acumul);
                   ^
common.cpp: In function 'std::__cxx11::wstring FromUTF8(const char*)':
common.cpp:69:61: warning: comparison is always true due to limited range of data type [-Wtype-limits]
                 else if(std::numeric_limits<wchar_t>::max() < 0x110000){
                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
db.cpp: In function 'int emptyCallback(void*, int, char**, char**)':
db.cpp:7:32: warning: unused parameter 'data' [-Wunused-parameter]
 static int emptyCallback(void* data, int argc, char** argv, char** azColName){
                                ^~~~
db.cpp:7:42: warning: unused parameter 'argc' [-Wunused-parameter]
 static int emptyCallback(void* data, int argc, char** argv, char** azColName){
                                          ^~~~
db.cpp:7:55: warning: unused parameter 'argv' [-Wunused-parameter]
 static int emptyCallback(void* data, int argc, char** argv, char** azColName){
                                                       ^~~~
db.cpp:7:68: warning: unused parameter 'azColName' [-Wunused-parameter]
 static int emptyCallback(void* data, int argc, char** argv, char** azColName){
                                                                    ^~~~~~~~~
db.cpp: In function 'int langGetCallback(void*, int, char**, char**)':
db.cpp:16:34: warning: unused parameter 'data' [-Wunused-parameter]
 static int langGetCallback(void* data, int argc, char** argv, char** azColName){
                                  ^~~~
db.cpp:16:70: warning: unused parameter 'azColName' [-Wunused-parameter]
 static int langGetCallback(void* data, int argc, char** argv, char** azColName){
                                                                      ^~~~~~~~~
db.cpp: In function 'std::__cxx11::string* langGet(std::__cxx11::string, int)':
db.cpp:33:9: warning: unused variable 'rc' [-Wunused-variable]
     int rc = sqlite3_exec(DB, sql.c_str(), langGetCallback, (void*)"", NULL);
         ^~
db.cpp: In function 'int configSingleGetCallback(void*, int, char**, char**)':
db.cpp:42:42: warning: unused parameter 'data' [-Wunused-parameter]
 static int configSingleGetCallback(void* data, int argc, char** argv, char** azColName){
                                          ^~~~
db.cpp:42:78: warning: unused parameter 'azColName' [-Wunused-parameter]
 static int configSingleGetCallback(void* data, int argc, char** argv, char** azColName){
                                                                              ^~~~~~~~~
db.cpp: In function 'std::__cxx11::string configSingleGet(std::__cxx11::string)':
db.cpp:57:9: warning: unused variable 'rc' [-Wunused-variable]
     int rc = sqlite3_exec(DB, sql.c_str(), configSingleGetCallback, (void*)"", NULL);
         ^~
db.cpp: In function 'int getLayoutCallback(void*, int, char**, char**)':
db.cpp:66:29: warning: unused parameter 'data' [-Wunused-parameter]
 int getLayoutCallback(void* data, int argc, char** argv, char** azColName){
                             ^~~~
db.cpp:66:65: warning: unused parameter 'azColName' [-Wunused-parameter]
 int getLayoutCallback(void* data, int argc, char** argv, char** azColName){
                                                                 ^~~~~~~~~
db.cpp: In function 'std::__cxx11::string getLayout(int)':
db.cpp:81:9: warning: unused variable 'rc' [-Wunused-variable]
     int rc = sqlite3_exec(DB, sql.c_str(), getLayoutCallback, (void*)"", NULL);
         ^~
db.cpp: In function 'int getLayoutDimCallback(void*, int, char**, char**)':
db.cpp:90:32: warning: unused parameter 'data' [-Wunused-parameter]
 int getLayoutDimCallback(void* data, int argc, char** argv, char** azColName){
                                ^~~~
db.cpp:90:68: warning: unused parameter 'azColName' [-Wunused-parameter]
 int getLayoutDimCallback(void* data, int argc, char** argv, char** azColName){
                                                                    ^~~~~~~~~
db.cpp: In function 'int* getLayoutDim(int)':
db.cpp:105:9: warning: unused variable 'rc' [-Wunused-variable]
     int rc = sqlite3_exec(DB, sql.c_str(), getLayoutDimCallback, (void*)"", NULL);
         ^~
db.cpp: In function 'int configUpdate(std::__cxx11::string, std::__cxx11::string)':
db.cpp:120:9: warning: unused variable 'rc' [-Wunused-variable]
     int rc = sqlite3_exec(DB, sql.c_str(), emptyCallback, (void*)"", NULL);
         ^~
db.cpp: In function 'int langGetCallback(void*, int, char**, char**)':
db.cpp:16:12: internal compiler error: in pp_format, at pretty-print.c:630
 static int langGetCallback(void* data, int argc, char** argv, char** azColName){
            ^~~~~~~~~~~~~~~

db.cpp:16:12: internal compiler error: Aborted
g++: internal compiler error: Aborted (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

In this moment I doubt the issue on the vector has any relation (although I appreciate that points), I tried to just remove all the calls to Vectors, Bidimensional arrays, etc; and still the constructor corrupts it (Now throwing a value of 0x8addd0 "�.")

Enrique López Sales
  • 43
  • 6
  • 1
    `layout_2d[current_row][current_col]`: Your inner vectors are all empty. You can't index at `current_col` into them. – user17732522 Apr 11 '23 at 22:23
  • @user17732522 so it means that when I initialized layout_2d i only initialized it to have `rows` positions of the vector of Room*; but the vector of Room* itself has no positions if I understand correctly? – Enrique López Sales Apr 11 '23 at 22:39
  • @EnriqueLópezSales: Change this: `layout_2d[current_row][current_col]` to this: --> `layout_2d.at(current_row).at(current_col)` -- Does that explain the error better? You will now get a `std::out_of_range` exception instead of corruption. – PaulMcKenzie Apr 11 '23 at 22:41
  • Yes, correct. The `std::vector` constructor you used _value-initializes_ `rows` elements. _Value-initialize_ means elements are initialized via their default constructor for class types like `std::vector`, which in turn initializes to an empty state. There is a `std::vector` constructor that takes the initial state for the elements as second argument, so something like `std::vector> layout_2d(rows, std::vector(cols));` will work. See https://en.cppreference.com/w/cpp/container/vector/vector. But see the posted answer for a better approach. – user17732522 Apr 11 '23 at 22:44
  • 1
    @EnriqueLópezSales FYI --`for (size_t i = 0; i < strlen(layout.c_str()); i++)` -- Why are you writing this very inefficient loop, when this: `for (size_t i = 0; i < layout.size(); i++)` is far better? The `strlen()` is a function call that needs to walk the entire string to figure out the size, and you're calling it each and every time the loop iterates. The `size()` is `O(1)`. – PaulMcKenzie Apr 11 '23 at 22:45
  • vectors of vectors can be slow because a `vector` is basically a pointer to an array of it's contents plus some book-keeping. That means a `vector` of `vector` is a pointer to an array of `vector`s and each of the inner `vector`s is a pointer to an array. This means the memory could be scattered all over storage. If you use one big vector and do the indexing math yourself you can get one big cache-friendly block. [Link to simple example](https://stackoverflow.com/a/36123944/4581301). Since there are no inner `vector`s to initialize this also solves the current problem. – user4581301 Apr 11 '23 at 22:55
  • Thanks for your comments, I tried some approximations like the ones offered by @user17732522 for the constructor of the vector and the one from @PaulMcKenzie for a better usage of the `for()` loop. Also tried approximately the option from @user4581301 for a 2D Vector, but using `Room` and `Room*` instead of int. All of them had some usage, but still the constructor corrupts the input variable into the same character – Enrique López Sales Apr 12 '23 at 19:42
  • @EnriqueLópezSales Then something else in your program is causing UB, e.g. something in your `Room` class. Compile and run your program with `-fsanitize=address,undeifned` if you can and the sanitizers may give you a more precise location for the cause. Also make sure you compile with `-Wall -Wextra` and fix every warning that you get. It is unfortunately not possible to do full debugging for your program in this setting (nor will most users want to do that). – user17732522 Apr 12 '23 at 19:46
  • @EnriqueLópezSales Also, from the information in your question I don't actually understand whether the debugger information you are seeing is actually a problem or not. – user17732522 Apr 12 '23 at 19:52
  • @user17732522 I launched the compiler with that parameters, I edited the output in the post. And as I added, I think is not related to vectors, because I removed all the vectors and the problem still happens – Enrique López Sales Apr 12 '23 at 20:01

1 Answers1

1

Looks like you have zero sized vectors in your vector:

std::vector<std::vector<Room*>> layout_2d(rows);  // Notice you don't use cols
                                                  // So there are no cols in each row!

Now you could sort this with:

std::vector<std::vector<Room*>> layout_2d(rows, std::vector<Room*>(cols));

But this leads to another issue. Your internal type is an unmanged pointer Room*. You may correctly delete all those rooms at some point (you don't show the code), but its probably not exeption safe.

I would change the type and the way you add rooms to make everythign safe:

std::vector<std::vector<Room>> layout_2d(rows);   // Notice Room NOT Room*

Then change your code to insert a room to:

    Room* r = new Room(acumul);
    layout_2d[current_row][current_col] = r;

    // Change to:

    layout_2d[current_row].emplace_back(acuml);
Martin York
  • 257,169
  • 86
  • 333
  • 562
  • Hello, I am looking at the second part of this but I cannot completely understand how this solves it; I mean, I need my layout_2d to be bidimensional having rows and cols; but I see that the change to using `emplace_back` looks like it just pushes a new string (`acumul`) into the layout_2d vector, not a Room – Enrique López Sales Apr 12 '23 at 19:22
  • when you call `emplace_back()` it expands the size of the vector by one and the calling the constructor of the newly added `` (in this case Room), passing the constructor all the parameters you provide (as many as you need) to the emplace_back call. So in this specific case it constructs one new `Room` object passing `acuml` to the constructor of `Room`. Note: the last change requires a change in the type of `layout_2d` as explained above. – Martin York Apr 12 '23 at 20:22
  • Ok, I see the point, I didn't knew the details of `emplace_back()`, seems useful. I updated the OP post in order to add more details, since it seems the issue on vectors has no influence, because I tried removing any vectors in the code and the same error in constructor still happens – Enrique López Sales Apr 12 '23 at 20:26