Code causing stack overflow imediatly after main

Question

So i wrote a driver for class that uses a menu to test out different hashing techniques. What i have is an abstract class "BaseHash" with all virtual functions and 5 different child classes that hash in different ways. When i do this:

while (numChoice!=0){
        switch(numChoice){
        case 1: myList= &MyHashContainer(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 2: myList= &hash2(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 3: myList= &chainingHash(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 4: myList= &quadraticHash(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 5: myList= &DoubleHash(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;

        }
    }

I get a stack overflow error the moment main is called. The debugger doesnt even let me get one step out before throwing the error.

If it matters myList looks like this

BaseHash *myList;

If i comment out the code starting with while, it does not throw this error. I dont even know where to begin with why this might happen.

If you get a crash before main you generally have an issue with some static that is getting intilized before main is called. — rerun, Jan 26 '13 at 01:17
Sorry guys i didnt think this one through all the way. Only ever worked with high level languages so sometimes i get messed up with the grittier details. Answer posted below — Mr. MonoChrome, Jan 26 '13 at 01:23
@Ukemi: one or more of those types is _far_ too big. Use `vector` instead of arrays, and try to keep all objects less than 64 bytes. — Mooing Duck, Jan 26 '13 at 01:39

score 1 · Accepted Answer · answered Jan 26 '13 at 01:22

1

I figured this out. I forgot that the stack has only a limited space on it, and i needed to use the "new" keyword to make sure all this was going on the heap

while (numChoice!=0){
        switch(numChoice){
        case 1: myList= new MyHashContainer(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 2: myList= new hash2(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 3: myList= new chainingHash(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 4: myList= new quadraticHash(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;
        case 5: myList= new DoubleHash(myStudentList.getStudentList(),myStudentList.getStudentList().size(),p);break;

        }
    }

answered Jan 26 '13 at 01:22

Mr. MonoChrome

1,383
3
17
39

This indeed solves your immediate problem but the reason for it isn't what you describe - see my answer above. – SomeWittyUsername Jan 26 '13 at 01:24
@icepack: Actually, it _is_ the reason for what he describes, and is _very_ related to the correct solution. – Mooing Duck Jan 26 '13 at 01:40
@MooingDuck no, it's not really the main reason, just a side-effect. This might be a correct solution for his situation (need more data to conclude that), but stack isn't the main problem here, the original code references addresses of temporaries, causing an undefined behavior; this is very **wrong**. – SomeWittyUsername Jan 26 '13 at 06:34
@MooingDuck: Actually, icepack is right. Although this is of course very bad C++ style, there is no technical reason why Ukemi could not have, say, `alloca()`'d enough space for the derived hash class with the greatest `sizeof()`, and then used placement `new` to initialize the appropriate hash object in each `case`. So this is not a stack vs heap problem. – isekaijin Jan 26 '13 at 07:05
@Eduardo: icepack is both right and wrong. icepack is correct that the code is wrong and bad and why. _However_, what icepack describes would cause a crash _after_ the switch executes. If it's having a stack overflow _as main begins_, thats usually because of what I described. (also, alloca creates stack issues, it doesnt solve them, but I assume you meant "allocated") – Mooing Duck Jan 26 '13 at 16:43
@MooingDuck: You are right that icepack's solution would cause a crash after the `switch` executes - each temporary hash object would be declared (and thus constructed and destructed) in a `{...}` block within a `case`, so it would have already been destructed when the control of flow exists the `switch`. However, I *did* mean `alloca()` in my comment. There is nothing wrong with using `alloca()` for small memory chunks that must be deallocated when the control of flow exits the function anyway. – isekaijin Jan 26 '13 at 18:42
So although the problem is solved most of the comments refer to this being bad style... im very up to correcting this if someone wants to inform me why This program is just a simulation program on hashing styles... it uses an interactive menu so that the user may choose the hash style at runtime. Im having it return said pointer to an abstract class... so that later my main can call virtual functions – Mr. MonoChrome Jan 26 '13 at 19:15
@MooingDuck here stack overflow is a side effect, on another compiler or even on the same one with different optimization options it won't necessary result in overflow because storage place of temporary objects isn't defined by the standard, see more here: http://stackoverflow.com/questions/9109831/where-are-temporary-object-stored – SomeWittyUsername Jan 26 '13 at 19:23
@icepack im not sure how this conversation relates to what should be changed about this code. Your saying that with different optimization settings all this could fit on the stack. Which is a fair point, but your posts make it sound like getting an address of a temporary object, is bad practice and should be changed. As a student my largest consern is how to change that practice and why its bad. And isnt that what using the "new" operator is all about. If storing the address of a temporary object is bad, why ever use "new"? – Mr. MonoChrome Jan 26 '13 at 20:00
1

@Ukemi the problem with using address of temporary object is that it, well, temporary - you can't rely that the address is valid since the memory can be freed or overwritten at any time without your intervention. `new` allocates a non-temporary space on the heap and stores the object there - that's the difference, the return value of `new` is the address of the allocated non-temporary memory. This memory should be released when non longer needed with `delete`, to avoid memory leak – SomeWittyUsername Jan 26 '13 at 20:08
@icepack So as long as i use new, then everything i did is ok practice, yes? – Mr. MonoChrome Jan 26 '13 at 20:33
add `delete` for each `new` – SomeWittyUsername Jan 26 '13 at 21:01
Ukemi: sorry to be confusing. We're all in agreement that using the new as you're using it here is the best solution (along with `delete`), we're just debating some details about what caused the crash in the first place. @icepack: there are safe and conforming ways to take the address of a temporary, namely if you store it by _const_ reference (but in practice that's virtually useless and nobody does it). But even if he did that part safely, or even used copies instead of pointers, I believe the overflow would still occur because I believe the problem is one or more of the objects is too big. – Mooing Duck Jan 26 '13 at 21:15
@MooingDuck The objects are exceptionally big... its 1600 studentss per hash, and each student has about 8 string data members attached. which is stored in a vector. – Mr. MonoChrome Jan 26 '13 at 21:38
@MooingDuck not sure how const reference helps here. Whatever you do, address of temporary is UB since temporary is an rvalue. – SomeWittyUsername Jan 26 '13 at 21:48
@icepack: It's a wierd quirk of the rules, but temporaries _can_ be bound to const reference, it's part of how we can pass temporaries to functions that accept a const reference parameter, but not to functions expecting a mutable reference: http://stackoverflow.com/questions/760578/const-reference-to-temporary – Mooing Duck Jan 26 '13 at 21:55

SomeWittyUsername · Answer 2 · 2013-01-26T06:45:05.737

1

You're assigning to myList an address of the temporary outcome of constructor execution - this value is a temporary that doesn't have an address associated with it, therefore the operation results in an undefined behavior. Storing the return value in a temporary variable and taking its address will resolve the technical problem, but there is probably a logical one as well - an attempt to take address of return value is most likely a logical mistake.

edited Jan 26 '13 at 06:45

answered Jan 26 '13 at 01:22

SomeWittyUsername

18,025
3
42
85

Taking the address of a return value is a perfectly legitimate operation if the return value is a reference. (btw, I am not the downvoter) – isekaijin Jan 26 '13 at 06:51
@EduardoLeón that's right, but in this case these are constructors that by definition produce a new object (not reference). – SomeWittyUsername Jan 26 '13 at 06:54
(OP says the code has a stackoverflow as it calls main, before any line executes. Yes this is an error in the code, but it is not the cause of his stackoverflow. – Mooing Duck Jan 26 '13 at 16:46

Code causing stack overflow imediatly after main

2 Answers2