Need to remove all direct uses of abort(). qFatal should be used instead.
For better debugability we need to remove all direct uses of abort(). qFatal should be used instead. There are about 60 direct uses of abort() which need to be removed. The preceding qDebug needs to be changed into a qFatal.
This will allow to catch all occasions where the application aborts with one single breakpoint, that is, in the proposed message handler: http://musescore.org/en/node/14748
Searching through the code to find debug messages and locate the abort() is not an efficient way to work.
P.S.: instead of calling qFatal, it would perhaps be better to use a pre-processor macro, so we get file and line number; something like:
#define WHERESTR "[file %s, line %d]: "
#define WHEREARG __FILE__, __LINE__
#define FATAL2(...) qFatal ( __VA_ARGS__)
#define FATAL(_fmt, ...) DEBUGPRINT2(WHERESTR _fmt, WHEREARG, __VA_ARGS__)
FATAL ("this count looks bad: %d\n", someCount);
(I copied this from Wikipedia , so I hope it works. If it doesn't, something similar can be made to work.)
Comments
While someone is at it, it would be good to have a DEBUG macro, too.
#define DEBUG2(...) qDebug ( __VA_ARGS__)
#define DEBUG(_fmt, ...) DEBUG2(WHERESTR _fmt, WHEREARG, __VA_ARGS__)
Oops, the original post had a cut/paster error:
#define FATAL2(...) qFatal ( __VA_ARGS__)
#define FATAL(_fmt, ...) FATAL2(WHERESTR _fmt, WHEREARG, __VA_ARGS__)
It is not 100% a bug, but more a kind of feature request.
But, terminology put aside, I second this proposal 100% !!
M.
Well, I understand "feature" to be a feature that the user can see.
Potentially exiting the application in 60 different places with abort() is bad practise, so I'd call this a bug.
But let's not discuss semantics here, let's get it done 100%!!
Bad practise? I dont see the difference between exiting the program with abort() and with qAbort(). abort() _is_ the single exit point. You should be able to set a breakpoint on abort() to catch it.
I also dont see the the benefit of the FATAL macro. You do have the stacktrace, dont you?
The added benefit of the macros are that they would print file and line number. If more qDebug would be used in the code, one could observe the application while it is doing it's work.
In the larger scale projects I have worked on, we used a single exit out of the application which was under our control. At that stage, additional crash information can be collected.
I agree that abort() is the single point of exit. Sadly, in the "broken" Windows environment it is impossible to set a breakpoint on a routine that is part of the Qt runtime. Neither is it possible to step into the abort() function. Therefore the Qt development community suggests a message handler, as we have now.
I would like to see all the aborts() interspersed in the code disappear. So:
qDebug ("this looks bad, oh boy!");
abort();
should be replaced by
qFatal("this looks bad, oh boy!");
Of course my preference would be a macro, as stated.
P.S.: Of course Windows offers a variety of debuggers: There is WinDbg and then there is a debugger in Developer Studio. With WinDbg you can set a breakpoint wherever you like, I assume, but that's a debugger for "real man". Some developers like myself (although coding in IBM assembler decades ago) have become quiche eaters, so we like it the easy way, using what the IDE has to offer (and work around the fact that some things can't be done). I have WinDbg on the machine, but I only use it to analyse Windows core/dump files when Windows crashes (which doesn't happen all that often). I trust you know the story about "real man" and quiche eaters, if you don't, here it is: //www.pbm.com/~lindahl/real.programmers.html
Yes, while abort() may work on some OS's, it is a big problem on Windows with QtCreator / gdb. Way too often I get a crash that the debugger won't catch and it turns out to be an abort() call. It's especially maddening because it's a problem (like a potential null dereference) that the code itself was detecting and *trying* to handle gracefully. But for programmers on Windows, calling abort() is *worse* than just dereferencing the null pointer, since the debugger would trap the latter and give me a meaningful stack trace. Whereas with an abort() call, the program just dies.
Even the existing mscoreMessageHandler is less than ideal, because while I can step a breakpoint on that call to abort() (something I wouldn't ordinarily think do do until a problem like this shows up, so there's some time lost already), I don't actually get a meaningful stack trace. I have to hope the message is sufficiently clear to show me where it got called, then re-do the whole session with a breakpoint where I'm guessing the problem actually occurred - more lost time.
It's a serious issue, and apparently has a simple fix.
Funny, when I created the message handler back in 2012 there was a useful stack trace, see enclosed.
![14835.png]( "14835.png")
Yes, I think I used to see that too. I don't any more. Do you?
I haven't tried in about two years. Since then Qt has changed a lot.
See PR #737
Fixed in 3adc1ea6ed
Automatically closed -- issue fixed for 2 weeks with no activity.
sorry for bump, but I'm in process of fixing bug #78556: Album segfault {2parts-excerpted}.append( {1part-notexcerpted} ); where a segfault occured after a NULL Measure* returned by tick2measure() was then dereferenced:
https://github.com/musescore/MuseScore/blob/master/libmscore/undo.cpp#L…
I know how to fix that bug. But any null dereference there (m->undoGetSegment()) produces a bogus stack trace, so as part of my PR but as part of the PR I wanted to include something between those to lines to handle condition that the measure was NULL. I was considering putting if (m==null) qFatal("Couldn't find measure, will not dereference NULL measure").
Could someone enlighten me on what is musescore coding practice for such a situation, and maybe add a coding rule regarding proper abortions to: https://musescore.org/en/developers-handbook/musescore-coding-rules
qFatal is fine. I know it's not a popular opinion in some circles but if possible, I would prefer to do nothing and not crash (which will be the case if qFatal is used)
ok. I will try to see if I can propagate up failure code to the call stack eventually to album::createScore()
In my segfaul, the call stack was:
undoAddCR currently returns void...I will have to add a return code to that, so I'll make that a boolean return false if fail. Also appendScore currently returns int index...I could return -1 for failure. Then chain of failure would be complete.