Memory leak problem, help appreciated

[xyzqm]

Hey guys,
So if you've read my code you might know that there are a couple memory leaks. I've been using C++ for around 2 years now, but pointers are not my forte. If you are more experienced, I would appreciate a couple tips to fixing these leaks, and possibly a pull request. Thanks!

[andrewgopher]

Use delete my_object to delete your object. If your object itself has pointers, you can have a ~ClassName() method to delete those, and if those object ALSO have pointers you can have destructors for those.

[andrewgopher]

I might make a pull request later.

[andrewgopher]

So I've been experimenting with adding a destructor for BinOp(). Here are my updated AstNode and BinOp defintions:

class AstNode {
  private:
  std::string type;
  public:
  AstNode () {};
  AstNode (std::string typePass) {
    type = typePass;
  }
  virtual~AstNode () {}
  virtual std::string print () = 0;
};
class BinOp : public AstNode {
  public:
  AstNode* left;
  Token op;
  AstNode* right;
  BinOp () {};
  BinOp (AstNode* leftPass, Token opPass, AstNode* rightPass) {
    left = leftPass;
    op = opPass;
    right = rightPass;
  }

  ~BinOp () {
    std::cout << "deleting.." << std::endl;
    delete left;
    delete right;
    std::cout << "delete successful!" << std::endl;
  }

  Token getOp () {
    return op;
  }
  std::string print () {
    std::string s = "BinOp";
    return s;
  }
};

I added a virtual destructor for AstNode and added a destructor for BinOp to delete the left and right pointers. I don't know where delete is called for an instance of BinOp, but the destructor is run a few times when testing it out on test.spark and successfully, too. This is outputted (I log the deleting of BinOp):

deleting..
delete successful!
deleting..
delete successful!
deleting..
delete successful!
deleting..
deleting..
Segmentation fault (core dumped)

Weird. Someone on stackoverflow says:

Segfaulting mean trying to manipulate a memory location that shouldn't be accessible to the application.
That means that your problem can come from three cases:
Trying to do something with a pointer that points to NULL;
Trying to do something with an uninitialized pointer;
Trying to do something with a pointer that pointed to a now deleted object;

[xyzqm]

Hm...
You said it was successful, but I see a segmentation fault. What do you mean by "successful?"

[andrewgopher]

"Successful" means that both the left and right pointers could be deleted without error.

I think the error occurs on deleting of left or right. I used print statements and neither pointer seems to be NULL on deletion, so we can cross that possibility out.
I don't think the pointer is already deleted, so the remaining suspect is deleting an uninitialized pointer, or a pointer that is created but never assigned to by new ClassName().

[andrewgopher]

Even weirder findings. When I run the following spark code (with the same c++ code):

main() {
    vars:
        int: a, c, d;
    program:
        a = 3;
        c = 5;
        d = 6;
        c = a - d;
}

The segfault isn't even in the destructor for BinOp. Output:

deleting..
delete successful!
Segmentation fault (core dumped)

Is the pointer for left and right used elsewhere, after the parent BinOp node is deleted?

[xyzqm]

Where did you delete the BinOp node?

[andrewgopher]

Hmm... I didn't delete the BinOp node anywhere new. I don't think the "garbage collector" would delete it.

Also, the BinOp node is deleted before the code is executed, so that's why there is a Segmentation fault.

[andrewgopher]

Ok, I've narrowed the error down to here:
return (visit<int>(binOp.left) - visit<int>(binOp.right));
This is in visit_BinOp, in the case where the binOp is of type Minus and the numbers being subtracted are int. (first case)

[xyzqm]

Hmm... I didn't delete the BinOp node anywhere new. I don't think the "garbage collector" would delete it.

Also, the BinOp node is deleted before the code is executed, so that's why there is a Segmentation fault.

You are right, I'm pretty sure C++ does not have built-in garbage collection.

[xyzqm]

Ok, I've narrowed the error down to here:
return (visit<int>(binOp.left) - visit<int>(binOp.right));
This is in visit_BinOp, in the case where the binOp is of type Minus and the numbers being subtracted are int. (first case)

What happens if it is of type Plus?

[andrewgopher]

If I change the minus in the test code

main() {
    vars:
        int: a, c, d;
    program:
        a = 3;
        c = 5;
        d = 6;
        c = a - d;
}

to a plus, the same error occurs: Segmentation fault while evaluating (visit<int>(binOp.left) + visit<int>(binOp.right)).

[xyzqm]

On a side note, are you using Visual Studio?
If so, there's a neat debugging feature you can use.

[andrewgopher]

Yes, I do use Visual Studio (Code). Please tell me how!

[xyzqm]

Oh, I'm not sure it works on Visual Studio Code.
I'm on Windows, so I use Visual Studio.
If you're on Mac, I'm not sure whether X-Code has support for debugging.
Here's an article you might want to check out: https://code.visualstudio.com/docs/editor/debugging#:~:text=To%20run%20or%20debug%20a,and%20save%20debugging%20setup%20details.
Breakpoint debugging is a bit annoying, but it works.

[andrewgopher]

I use kubuntu (linux), XD.

I'm checking out the debug tab and the .vscode/launch.json file.

[xyzqm]

Ah yes, Linux, the OS that all programmers use xD
An alternative to Visual Studio and XCode would be Eclipse. VSCode is rather complicated, but it works. 😄

[andrewgopher]

Good debugger! Pauses on the segmentation fault and shows the call stack at that point.

[xyzqm]

Yep, that's exactly how it works in Visual Studio.
If you click on functions in the call stack you should be able to see details such as parameters passed.

[xyzqm]

I see.
Have you learned anything from just looking at the call stack though?

[andrewgopher]

It uses the same debugger actually, but I can't see the parameters through clicking.

I already know the call stack from using print statements though.

[andrewgopher]

It doesn't show the variables either, unfortunately.

[andrewgopher]

According to the call stack, when running the default test.spark file (not the simplified one I made), SematicAnalyzer::visited(AstNode*) calls the destructor for BinOp (aka ~BinOp()).

[xyzqm]

visited or visit?

[andrewgopher]

Oops LOL I am really used to saying visited because of competitive programming (visited arrays for graphs)
Yes, I mean visit

[andrewgopher]

🥳 I've found the error! When I run the following code:

main() {
 	vars:
		int: a, c, d;
	program:
		c = 5;
		d = 7;
		a = (3 + 6) // 3;
}

the BinOp for the plus sign is deleted two times, meaning the second time will cause a segmentation fault. Now I just need to find where this BinOp is deleted and created again.

[andrewgopher]

BTW, I outputted the op type when starting the destructor and the output is:

deleting +
deleted
deleting //
deleting +

[xyzqm]

Nice!
Thanks for the help 😃

[andrewgopher]

BTW, don't use pointers unless you need to have the same copy of memory. Even if you do, you can use references instead. But if you REALLY need to use pointers, I would recommend smart pointers which automatically delete themselves.

[xyzqm]

You are right, I forgot all about smart pointers.
The only reason I had to use pointers was that the AstNode type had to be able to contain all possible nodes, which is impossible to implement without inheritance and such (at least to my knowledge). I experimented with union but it didn't work.

[andrewgopher]

I'm a cpp noob so I don't know what a union is -_-.

I read that the OS reclaims the memory when the process is done, so actually if you aren't creating pointers forever (example: a web server that runs for a long time) you don't necessarily need to delete your pointers. I'm still going to try to optimize memory, though.

[andrewgopher]

As you might have read in my pull request, I finally have my debugger fully working!
The call stack to the breakpoint after the first destructor:

BinOp::~BinOp(BinOp * const this) (/home/andrew/Downloads/MakeALanguage-master/Parser/astnodes.h:31)
SematicAnalyzer::visit(SematicAnalyzer * const this, AstNode * node) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:68)
SematicAnalyzer::visit_BinOp(SematicAnalyzer * const this, BinOp binOp) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:140)
SematicAnalyzer::visit(SematicAnalyzer * const this, AstNode * node) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:68)
SematicAnalyzer::visit_Assign(SematicAnalyzer * const this, Assign & assign) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:151)
SematicAnalyzer::visit(SematicAnalyzer * const this, AstNode * node) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:74)
SematicAnalyzer::visit_Block(SematicAnalyzer * const this, Block block) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:133)
SematicAnalyzer::visit_Program(SematicAnalyzer * const this, Program & program) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:95)
SematicAnalyzer::visit(SematicAnalyzer * const this, AstNode * node) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/SymbolTable.h:62)
interpreter::Interpreter::interpret(interpreter::Interpreter * const this) (/home/andrew/Downloads/MakeALanguage-master/Interpreter/interpreter.h:368)
main(int argc, char ** argv) (/home/andrew/Downloads/MakeALanguage-master/main.cpp:78)

Ok, so the first time ~BinOp() is called, it's from the visit method in SematicAnalyzer. According to my debugger, the latest call of visit is on the right node (the parameter for node is right). This is the right pointer owned by the BinOp being deleted.

In other words, when visiting the right of a BinOp, the BinOp's destructor is called somehow on the line where visit_BinOp is called with the casted version of that right node.

[andrewgopher]

Oh yeah, I forgot to tell you the test code I ran:

main() {
 	vars:
		int: a, c, d;
	program:
		c = 6;
		a = 3;
		d = 3;
		a = (c - d) // 3;
}

[andrewgopher]

Another thing I forgot to tell you: the BinOp being deleted was the minus sign

[xyzqm]

I'm a cpp noob so I don't know what a union is -_-.

I read that the OS reclaims the memory when the process is done, so actually if you aren't creating pointers forever (example: a web server that runs for a long time) you don't necessarily need to delete your pointers. I'm still going to try to optimize memory, though.

I did realize that, but if the code is very long memory will likely need to be released.

[andrewgopher]

True.

[andrewgopher]

You are right, I forgot all about smart pointers.

True, I don't think the static_cast you do doesn't work on normal variables.

I did realize that, but if the code is very long memory will likely need to be released.

It seems that the only memory we need to clean up is the objects generated by SematicAnalyzer, because it is not needed for the whole program.

[andrewgopher]

More progress! With the help of my dad, I figured out that the "copy constructor" was being called when using static_cast. That means we can have an attribute called is_copy_constructor and only delete left and right attributes if it is false.

Explanation: copy constructors copy the data from one pointer to another. However, the parent addresses are different. Because it goes out of scope, the new pointer made using static_cast is deleted, and so is left and right. However, we don't want this behavior as we will need left and right later. Just check if this pointer was made not using copy constructor before deletion.

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

[xyzqm]

We could also change the implementation to just directly use pointers instead of static_cast.
It's tedious, but it might work.
Your way is simpler though.

[andrewgopher]

What would you think of using smart pointers?

[andrewgopher]

It just changes the way the pointers are created, the operators used are the same.

[xyzqm]

It would definitely make for cleaner code 😄