Segfaults copying QStrings in simple calculator

Kenshi

I'm designing my first proper QT application. At some point, I'd like to go to Android, but for now I'm working in Linux.

I watched a guy design a calculator in QT on Youtube, but to make things a bit different, I'm making mine an RPN calculator as I vastly prefer. (If you don't know what RPN means, it's not that important in this context. It uses a stack like a computer processor if that helps.) Here is a copy of the two routines I think may be relevant.

void Calculator::NumPressed(){
    QPushButton *Button = (QPushButton *)sender();
    ButtonName = Button->text();
    ButtonName.remove(QChar('&'), Qt::CaseInsensitive);
    if(ReplaceLast == true){
        DisplayValue.clear();
        DisplayValue = ButtonName;
        ReplaceLast = false;
    } else {
        DisplayValue.append(ButtonName);
    }
    CalcStack[0] = DisplayValue.toDouble();
    Calculator::RefreshScreen();
}

void Calculator::EnterPressed(){
    for (int i = 8; i >= 0; i--){
        CalcStack[i+1] = CalcStack[i];
    }
    ReplaceLast = true;
    Calculator::RefreshScreen();
}

So at first, things work well. It lets me type in a number of one to several digits. Unlike the tutorial that uses a QLineEdit, I made a QTextEdit widget that shows the top three registers (very useful with RPN), so I start off with a 0 on each line, and as I type numbers, the bottom number changes, exactly like I planned.

Besides the numeric buttons, the only button I've tried to implement so far is the Enter button. The enter button is supposed to push everything up in the stack, in this case, CalcStack[8] is copied to CalcStack[9], then CalcStack[7] is copied to CalcStack[8], etc., until everything is moved up and the first two are the same. Again, this is exactly what should happen and I can do it multiple times.

The problem comes after I press Enter and then start typing a second number. That's when I get a seg fault. (At least I think it's a seg fault. Now when I try it, it tells me more ambiguously that it crashed.) As you can see, pressing Enter sets ReplaceLast to true, because if I start typing another number, I want it to start over rather than appending to the last one, though I don't want to get rid of it otherwise. For instance, in RPN, it's common to hit some number, press Enter, then press the multiply button to get its square, since the button for square sometimes requires pressing shift and is out of the way.

Anyway, this seems like it's going on and on but I'm almost finished. As you can see in the NumPressed routine, if ReplaceLast is true, the value in the top register (or really the QString representing it for the moment) is set to equal the button text, minus the ampersand. When I first ran it in gdb to see why it was crashing, it crashed right at "DisplayValue = ButtonName;". Then I added the clear function above that and it started crashing there instead. Right before it crashes, DisplayValue appears to be some kanji character (not by my intended design) while everything else appears to be just as expected.

ReplaceLast is initiated as true, so the very first number I type in goes through that same part of the if statement. For the life of me, I can't figure out why the first number works fine, and pressing Enter pushes the calculator stack as expected, but once I type another digit, crash... Keep in mind that I'm mediocre at C++; most of my experience is with C and assembly. Many thanks if you've managed to read through all this and many more if you can offer advice. I'm brand new with Qt.

Kenshi

Oh, it might also help to see the declarations and initializations of the variables used so you know what they are.

QString ButtonName;
double CalcStack[8] = {0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0};
QString DisplayValue = "0";
bool ReplaceLast = true;

sierdzio

@Kenshi said in Segfaults copying QStrings in simple calculator:

That's when I get a seg fault. (At least I think it's a seg fault. Now when I try it, it tells me more ambiguously that it crashed.)

Run your app in debug mode, with debugger attached. It will show you exact line where crash occurs.

for (int i = 8; i >= 0; i--){
  CalcStack[i+1] = CalcStack[i];

CalcStack has 8 values, so the topmost index is 7. This loop will access CalcStack[9] and CalcStack[8], which is likely to cause a crash (you are accessing memory outside of CalcStack array).

Kenshi

@sierdzio said in Segfaults copying QStrings in simple calculator:

Run your app in debug mode, with debugger attached. It will show you exact line where crash occurs.

I have done exactly that. I talked about it in my second to last paragraph, although I said I ran it in gdb when I meant the built-in debug mode. (I recently used gdb for another program written in assembly.) First, it was the "DisplayValue = ButtonName;" line in the if statement in NumPressed(), but again only when typing the second number. Then I decided to add the line before it, "DisplayValue.clear();" to try to make it more like when I'm typing the first number, but it started crashing on that line instead. It's as if something is happening to DisplayValue when I press Enter that utterly confuses any routines using it.

for (int i = 8; i >= 0; i--){
  CalcStack[i+1] = CalcStack[i];
CalcStack has 8 values, so the topmost index is 7. This loop will access CalcStack[9] and CalcStack[8], which is likely to cause a crash (you are accessing memory outside of CalcStack array).

You are absolutely right about that. I remember deciding I was going to use 8 registers since it's enough to easily do pretty much any calculation, but not so much I can't fill up the stack intentionally for certain tricks. Then I probably wrote that function and falsely remembered that I was using 10 registers.

I am absolutely dumbstruck, but that fixed my problem. By going outside the bounds of my array, I must have been editing DisplayValue in some way that confused those routines. I'm kind of surprised that DisplayValue.clear() even crashed it, but hey, after days of not making any progress on this, I'm suddenly back in the game after fixing such a rookie mistake I should have caught quickly.

I am very thankful for your keen observation (keener than mine anyway). I'd buy you a beer or six if I could. Now hopefully I can get the other buttons working and start adding more, make it somewhat like my HP32sii.

mrjj

Hi
If i may suggest something:
instead of having
double CalcStack[8] = {0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0};

then do
(#include <array>)

std::array<double,8> CalcStack {0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0};
then you can do
for (int i = 0; i < CalcStack.size() ; ++i) {
...
}

and you can change CalcStack at any time and code will still work.
Also works better as a parameter for a function and it has the same runtime performance as a plain c array.

so unless you cant use c++11, there is rarely any benefit of using raw c arrays anymore.

sierdzio

@Kenshi said in Segfaults copying QStrings in simple calculator:

I am very thankful for your keen observation (keener than mine anyway)

Heh, I made similar mistakes many, many times. And I keep doing them :-) I guess it's not about keen eye as such but rather a fresh, unbiased look of a different person. We tend to get "entangled" in our code and we focus on big things that we think are prone to errors. We often omit "trivial" code when looking for errors - because it's trivial so it must be good ;-)

I have done exactly that. I talked about it in my second to last paragraph, although I said I ran it in gdb when I meant the built-in debug mode. (I recently used gdb for another program written in assembly.) First, it was the "DisplayValue = ButtonName;" line in the if statement in NumPressed(), but again only when typing the second number. Then I decided to add the line before it, "DisplayValue.clear();" to try to make it more like when I'm typing the first number, but it started crashing on that line instead. It's as if something is happening to DisplayValue when I press Enter that utterly confuses any routines using it.

That's one "gotcha" about debuggers (and programs in general): they will happily go through and use invalid memory until they hit a brick wall.

So when debugger gives you crappy or random results - you can do 2 things:

run with address sanitizer
check if all objects leading into crashing code are valid (including, most confusingly, this object)

Kenshi

@mrjj said in Segfaults copying QStrings in simple calculator:

Hi
If i may suggest something:

Interesting. Like I said, I know C a lot better and I'm still learning a lot about C++. (I considered going through a tutorial on it first to refresh myself, but didn't have the patience.) Any recommendations are welcome.

Changing the stack size wouldn't be something likely done on the fly, but different people would likely have different preferences which might even depend on the situation, so I could see there being an options menu that let you set stack size. For instance, my 32SII has only 4 registers while my 48gII has 256 if I remember correctly. If I, for instance, wanted to count powers of 2, it would be easy on the "inferior" 32SII. I simply press 2, then press Enter three times so all registers contain 2, then repeatedly press the multiply button to my heart's content. The downfall is with so few registers, problems with lots of parentheses and various orders of operations can be pretty challenging.

I like your suggestion. I'm wondering though, if I set up an options menu and had an option to vary the number of registers, is there a way to initialize them based on a variable? For instance, if I used n to hold the number of registers, would the declaration

std::array<double, n> CalcStack;

work? If so, what would be the easiest way to initialize them to 0? A for loop?

mrjj

@Kenshi
Hi
The n most be a const expression so it cannot be dynamic.
const int n=10;
std::array<double, n> test;

however, if you need dynamic sizes, why not just vector ? ( or QVector)

std::vector<double> CalcStack(8); // 8 in list. set to zero

then you can do just add new doubles with ~~append~~. push_back
and list will grow.

Kenshi

@sierdzio said in Segfaults copying QStrings in simple calculator:

Heh, I made similar mistakes many, many times. And I keep doing them :-) I guess it's not about keen eye as such but rather a fresh, unbiased look of a different person. We tend to get "entangled" in our code and we focus on big things that we think are prone to errors. We often omit "trivial" code when looking for errors - because it's trivial so it must be good ;-)

Well in my defense, for what it's worth, I probably considered how many registers I wanted to use, and ended up deciding on 8, but at some point considered 10, so I flipflopped when writing it and they both looked good when glancing at it. Then there's the fact that I'm using totally new object types to me, so I'm hyper focused on these QStrings and such and hardly paying attention to the arrays that I should have down.

Since solving that problem, I've roughly doubled how many buttons are functioning, though I'm soon going to have to crash out. It's dawn and I'm going to have to go to work in the afternoon, but it's so hard to pull myself away while I'm finally making some major progress.

Kenshi

@mrjj said in Segfaults copying QStrings in simple calculator:

@Kenshi
Hi
The n most be a const expression so it cannot be dynamic.

I was afraid it would have to be a constant. Does the same apply to vectors? I've only recently started learning about them. Although I do seem to recall it is possible to declare a constant on the fly, like pulling the number of registers from options as n and then doing something like:

const int num_of_registers = n;
double registers[num_of_registers];
for (int i = 0; i < num_of_registers; i++)
    registers[i] = 0.0;

mrjj

@Kenshi
Hi
Yes it was allowed at some point to use a variable for C array definition.
I forgot when as i used mostly std::vector and friends for years.

No std::vectors or QVector are 100% dynamic.
They don't have a fixed size and allows them to be dynamically allocated and
expanded on the fly.
They are not as fast as raw c array but for many, many use cases that won't matter and
they offer so much more than c array.

I know its a bit odd at first when used to raw C array but you should really consider using
std to get max benefit from c++

Kenshi

Since I first posted here, I've gone from having the 10 numeric buttons and the enter button working somewhat to having all 20 buttons working almost precisely like my actual calculator. I have a crapload more I want to add, but I think I'm going to follow the suggestion of brushing up on some C++ features before I do. I'm sure I can code most of the stuff I want to at this point occasionally looking up functions and such, but it would likely be a bit sloppy and less than optimal.

Even though I started this as a learning exercise, I'm starting to get a little more serious about it and might want to try to port this to my phone when I figure all that out. It's much easier to make changes to some of the core methods by which it works while it's small than go through dozens of pages changing all kinds of things and trying to make it all work together as I've learned all too well at work. Plus now that I've gotten a little of the functionality in place, I have a better idea which way I want to go with it.

I greatly appreciate everyone's advice. I was about ready to tear my hair out (if I had any) before I finally got it to quit crashing. The last few hours, I've been zipping through the stuff. I will improve my C++. (It's been a few years since I studied it at all.) And hopefully some day I'll make something more impressive than a calculator. Expect to see me around either way. It's way past bed time so I'm gonna go crash out.