Strings getting deleted at the wrong time in c++

[SomeGuyWithAKeyboard]

I'm trying to understand the deconstructor in c++. I thought I understood how deconstructors work but turns out I don't.

I have the following pseudo code:

Code: Select all

do_the_string_bug()
{
	string var1 = "var1";
	printString(var1);		//var1 prints to screen correctly but the deconstructor get called when it returns
	printString("...");		//"..." prints to the screen correctly
	printString(var1);		//since the char array inside var1 has been deallocated and since "..." was the next string that got allocated, it now prints "...1" because that memory was overwritten
}
void printString(const string text)
{
	for (int i = 0; i < text.length(); i++)
	{
		printChar(text.at(i));
	}
}

In my program, once the code gets to the end of the print string function, it deletes the string that was passed to it causing the char array inside it to get deallocated. This means that other data can overwrite it causing all kinds of nasty bugs. I can remove the code that frees the char array. This stops the weird string bugs I keep having by preventing the printString function from being able to delete them but then every string operation becomes a memory leak since strings can no longer be deleted when the function that actually created the strings returns.

Why are functions calling the deconstructor on strings that get passed to them despite the const keyword?

[kzinti]

1) They are called destructors, not deconstructors.
2) You pass the string by value, not reference. This means the string is copied when you call the printString() function.
3) At the end of printString() the copied string (text) gets deleted because it goes out of scope.
4) The original string's destructor doesn't get called until the end of do_the_string_bug(). What I think is happening here is that you have not implemented a copy constructor for string and so the raw data pointer is copied as is. So you now have two different strings pointing to the same data.
5) "const" doesn't do anything at runtime, it's purely a compile time construct that has no impact on code generation. It cannot detect logic errors in your code at runtime. It certainly cannot prevent a destructor from being called.

You need to do something about copying strings:
1) Make copy unavailable.
2) Implement a copy constructor that copies the data (easier, less efficient).
3) Implement reference counting for the raw data (more difficult, more efficient).

You also probably most likely want to pass the string by reference and not by value.

[SomeGuyWithAKeyboard]

Ok thanks. I did experiment with copy constructors and found that a lot more stuff is likely messed up than I thought. No idea how I've gotten so far if that's the case, maybe I just got incredibly lucky to have everything string related mostly work except for this new bug I've only recently discovered. I tried substituting my memory allocator for the liballoc one which only causes it to crash sooner than before when I run it with the new copy constructor. Normally it works the same and gives me the same bug (but it crashes during keyboard scancode setup, a thing that doesn't crash when using my memory allocator) Would be nice if there was a c++ dynamic array that that uses no outside libraries so I could substitute that in and see if my vector equivalent class is broken or not but there isn't one.

Code: Select all

string :: string(const string &str)
{
    //charArray.clear();
    //charArray.resize(10);
    for (int i = 0; i < str.length(); i++)
    {
        charArray.push_back(str.at(i));
    }
}

string& string :: operator=(const string& other)
{
    //charArray.clear();
    //charArray.resize(10);
    for (int i = 0; i < other.length(); i++)
    {
        charArray.push_back(other.at(i));
    }

    return *this;
}

This is what I'm pretty sure the most correct implementation of the string copy constructor is. charArray is a dynarray<char> object that works exactly the same as vector.

It should also be noted that in my system, I need to be able to manually define where in memory allocated memory should be stored. For example I need stuff to be saved at 0x20000 and up instead of some random location beyond my control that's probably conflicting with stuff. I think liballoc_alloc is intended to be used for this but I still haven't figured out how it is meant to be used. The best hack I could come up with was making liballoc_alloc always return (void*)0x20000 since it only ever seems to call liballoc_alloc if there are no tags or pages already in memory and it allocates the next block based on where the previous one is. It doesn't seem to be writing things to anywhere besides location 0x20020 though.

[Octocontrabass]

Would be nice if there was a c++ dynamic array that that uses no outside libraries so I could substitute that in and see if my vector equivalent class is broken or not but there isn't one.

I'm sure it's not trivial, but you can always try ripping the std::vector implementation out of libstdc++. That should only rely on other libstdc++ pieces and a working malloc.

It should also be noted that in my system, I need to be able to manually define where in memory allocated memory should be stored.

That's usually how it works in an OS kernel.

I think liballoc_alloc is intended to be used for this but I still haven't figured out how it is meant to be used.

It's meant to hook into your page allocator. In a typical OS using paging, that would be a call to the virtual memory manager to find an appropriate range of virtual addresses and populate them with pages allocated via the physical memory manager.

The best hack I could come up with was making liballoc_alloc always return (void*)0x20000 since it only ever seems to call liballoc_alloc if there are no tags or pages already in memory and it allocates the next block based on where the previous one is.

It needs to return free memory (or NULL) every time it's called. I don't think liballoc will behave very well if you give it the same address twice.

[kzinti]

Using a dynamic array like std::vector<> and doing push_back() to copy strings one character at a time is not going to be very efficient. It would be better to just malloc a block of memory to hold the whole string and use memcpy() to copy the characters.

Code: Select all

string::string(const string& other)
{
    size = other.size();
    data = (char*)malloc(size+1); // +1 for the '\0' string terminator
    memcpy(data, other.data, size+1); // ditto
}

[davmac314]

Would be nice if there was a c++ dynamic array that that uses no outside libraries so I could substitute that in and see if my vector equivalent class is broken or not but there isn't one.

https://github.com/davmac314/libbmcxx

It's not much, but it has a basic `std::vector` implementation.

[Solar]

Obligatory links to the Wiki, Required Knowledge item 5 (Programming Experience), and Beginner Mistakes, "A hard truth".

If C++ destructors, the rule of three / five / zero, and passing by value vs. passing by reference throw you off course, you should re-evaluate if you really want to learn about those in kernel space (hint: you do not). Things will get a lot more complicated than that. A standalone "printString()" function does not speak for C++ fluency either. No offense intended, these are mistakes that are in the Wiki for a reason.

[Jiyahana]

Pass strings by reference in your functions.

Code: Select all

void do_the_string_bug()
{
   string var1 = "var1";
   printString(var1);      // var1 prints to screen correctly without invoking destructor
   printString("...");     // "..." prints to the screen correctly
   printString(var1);      // var1 prints correctly again without memory overwrite
}

void printString(const string& text)
{
   for (int i = 0; i < text.length(); i++)
   {
      printChar(text.at(i));
   }
}