#1241 closed defect
ALL: Crash while exiting
| Reported by: | SF/arbonline | Owned by: | SF/jamieson630 |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Here is the backtrace:
#0 0x40287c11 in __kill () at __kill:-1 #1 0x400956e3 in pthread_kill (thread=6508, signo=0) at signals.c:65 #2 0x400959eb in __pthread_raise (sig=1074384064) at signals.c:187 #3 0x402877b8 in *__GI_raise (sig=6508) at ../linuxthreads/sysdeps/unix/sysv/linux/raise.c:34 #4 0x4028937d in *__GI_abort () at ../sysdeps/generic/abort.c:88 #5 0x4020bd17 in __cxxabiv1::__terminate(void (*)()) (handler=0x402891e0 <*__GI_abort>) at ../../../../gcc-3.3/libstdc++-v3/libsupc++/eh_terminate.cc:47 #6 0x4020bd54 in std::terminate() () at ../../../../gcc-3.3/libstdc++-v3/libsupc++/eh_terminate.cc:57 #7 0x4020c327 in __cxa_pure_virtual () at ../../../../gcc-3.3/libstdc++-v3/libsupc++/pure.cc:50 #8 0x080d0a38 in MidiChannel_MPU401::controlChange(unsigned char, unsigned char) (this=0x6, control=123 '{', value=0 '\0') at sound/mpu401.cpp:54 #9 0x080d0ee5 in MidiChannel_MPU401::allNotesOff() (this=0x0) at sound/mpu401.h:69 #10 0x0807d9a1 in Part::silence() (this=0x81b10c8) at scumm/imuse.cpp:1629 #11 0x0808051c in Player::send(unsigned) (this=0x81b09c8, b=0) at scumm/imuse_player.cpp:261 #12 0x080ccfba in MidiParser::allNotesOff() (this=0x81b10c8) at sound/midiparser.cpp:243 #13 0x080cd830 in ~MidiParser_SMF (this=0x81b09c8) at sound/midiparser.h:237 #14 0x0807fec7 in ~Player (this=0x81b09c8) at scumm/imuse_player.cpp:78 #15 0x0807adb2 in ~IMuseInternal (this=0x81b08a0) at scumm/instrument.h:55 #16 0x0807dc2a in ~IMuse (this=0x81b08a0) at scumm/imuse.cpp:1713 #17 0x08051397 in ~Scumm (this=0x8192280) at scumm/scummvm.cpp:714 #18 0x080a26fe in ~Scumm_v6 (this=0x8192280) at scumm/intern.h:528 #19 0x080b37a9 in main (argc=0, argv=0x0) at common/main.cpp:236 #20 0x40273c17 in __libc_start_main (main=0x80b32ba <main>, argc=3, ubp_av=0xbffffc54, init=0x80d0f40 <__libc_csu_init>, fini=0x80d0f70 <__libc_csu_fini>, rtld_fini=0x40014140 <_rtld_local>, stack_end=0x4009ccc0) at ../sysdeps/generic/libc-start.c:152
Ticket imported from: #811092. Ticket imported from: bugs/1241.
Change History (20)
comment:1 by , 21 years ago
| Status: | new → pending |
|---|---|
| Summary: | Crash while exiting → ALL: Crash while exiting |
comment:2 by , 21 years ago
comment:3 by , 21 years ago
Version: ScummVM 0.5.1 (Sep 21 2003 14:11:26) It happens while using samnmax talkie version, at start using save and terminate or somthing like these but I can't reproduce. gcc 3.3. Thins go wrong at #8 when a pure virtual method is called and a excepton is thrown.
comment:4 by , 21 years ago
| Status: | pending → new |
|---|
comment:5 by , 21 years ago
| Owner: | set to |
|---|---|
| Status: | new → pending |
comment:6 by , 21 years ago
The following additional information is needed:
* Language of the game * Platform (Linux, BeOS, Solaris...) * Last version without the bug.
In particular, does this bug manifest itself in ScummVM 0.5.1 release (not CVS) or 0.5.0 release?
The backtrace includes some suspicious this pointers (why are Player and MidiParser_SMF getting the same this pointer??), and a null this pointer that could only occur as a result of a race condition. Might be a nuance in the mutex deletion during the destructor sequence.
Try changing line 1764 of scummvm/scumm/imuse.cpp (the IMuse::~IMuse() destructor) to the following:
IMuse::~IMuse() { in(); if (_target) delete _target; out(); _system->delete_mutex(_mutex); }
Of course, if you can't reproduce this anyway...... :/
comment:7 by , 21 years ago
I thought the issues with closing the MIDI driver had been fixed long ago, but maybe not. Could there be any relation to BASS bug #805923, "Notes left on when quitting game"?
I'm assigning this to Jamieson630, since he seems to know a lot about this kind of things. (But feel free to unassign yourself if I'm mistaken!)
comment:8 by , 21 years ago
| Status: | pending → new |
|---|
comment:9 by , 21 years ago
That's funny -- we both must have posted to this at the same time. WHY CAN'T I SEE YOU ON IRC, ERIK?! Tsk tsk tsk. ;)
comment:10 by , 21 years ago
| Status: | new → pending |
|---|
comment:11 by , 21 years ago
| Status: | pending → new |
|---|
comment:12 by , 21 years ago
Version is ScummVM 0.5.1 running under linux with sdl and alsa midi output. Game version is spanish. As I can't reproduce it I can't tell you the last version without the bug or if the code change works. sorry. I found this in internet about pure virtuals may help:
The C++ standard does not permit virtual methods to be called from constructors or destructors. In many cases older compilers had ignored this, since often the virtual table is populated anyway before the constructor is called, but that is implimentation specific behavior and should not be relied upon. Hence, things that can invoke virtuals cannot be specified in constructors or destructors either.
comment:13 by , 21 years ago
If the C++ standard says not to rely on virtual methods in destructors, then I agree we need to migrate the code toward something more compliant. On the other hand, you're using gcc on Linux, a configuration that plenty of other people use. Since neither they nor you can reproduce the problem, it does not appear to be the "fault" of an anally compliant compiler. (Such a compiler would be expected to nab the problem at compile time, anyway, not run time.) Since the problem appears to be inconsistent and extremely rare, the anomolies in your backtrace lead me to think it may have been a race condition.
For now, I will have to close this. If someone figures out how to reproduce the problem in a reasonably consistent manner, we can reopen the issue as a new bug post. In the meantime, I'll see about migrating the shutdown code for iMuse (and other qualifying MusicEngine derivatives) to more closely adhere to the standard. Lord knows, we've have enough problems with that shutdown sequence in the past.
comment:14 by , 21 years ago
| Resolution: | → worksforme |
|---|---|
| Status: | new → closed |
comment:15 by , 21 years ago
| Resolution: | worksforme |
|---|---|
| Status: | closed → new |
comment:16 by , 21 years ago
Just to clarify this, the problem with calling virtual functions from constructors / destructors is not the virtual method table (at least to the best of my knowledge and experience). Rather, it has semantic reasons (very valid ones, too): At the time a constructor runs, the object has only been setup up to the "generation" of the class the constructor was defined in. Assume now the object actually is an instance of a subclass of that class (i.e. a later generation), and thus more construtors are to be run. The point is, those additional constructors have not yet run. Thus the object has not yet been fully constructed. Hence the whole object is not yet truly an instance of its actual class. If you invoke a virtual method, the implementation available at that level of the class hierarchy will be invoked.
An example: Assume class A has a virtual method foo() which prints the letter "A", and B is a subclass of it. B overrides foo() to print "B". Now, calling foo() from the constructor of A will *always* print "A", even if the object being constructed is actually of class B
Similiar issues arrise for the destructor, just in reverse order.
So, calling virtual functions from constructors/destructors is actually legal (to the best of my knowledeg that is - at least I don't know any C++ compiler where it doesn't work in exactly the way I described above).
Anyway: this is not something up to compiler vendors. It's fixed in the C++ spec. And GNU C++ adhers to it in this regard (if you don't believe me, it only takes 5 minutes to write a program to verify this). That everything "works" is only a coincidence, Jamieson. The proper fix is to perform any deinit work involving virtual methods *before* deleting the midi parser object. E.g. add a deinit() method and call it just before deconstructing the parser.
comment:17 by , 21 years ago
| Status: | new → closed |
|---|
comment:18 by , 21 years ago
Termination sequence is now in a separate method that is called just before object destruction. Thanks for the lesson on virtual functions, Fingolfin, I never knew all that.
comment:19 by , 21 years ago
Err, no, this was a misunderstanding and the change you commited really wasn't necessary :-). You see, terminate() *never* even was virtual (to be virtual, it would have to be declared explicitly virtual. Hence, calling it from your destructor is perfectly fine. The same goes for the calls to in()/out() etc. - they are all fine and safe.
Rather I assumed you guys were referring to the *midi parser* destructor, which does a note off on all channels (or rather, tries to, but fails). Since the crash is also passing through the MidiParser destructor and the virtual MidiParser::allNotesOff(), that made a little sense. But looking through the code in sound/, it seems none of the subclasses override that, so just making it non- virtual should be fine, no?
Anyway: the crash still is extremly fishy since as you properly pointed out, the pointers to the IMuse and IMuseInternal objects are identical, which seems very odd...
Sorry for not putting this clear the first time (it's bad to silently assume what you think others *may* think and then go on from that, obviously :-(
I am not convinced this is really fixed, but unless somebody can reproduce it again, there is precious little we can do to track down the real cause.
comment:20 by , 5 years ago
| Component: | --Unset-- → --Other-- |
|---|
To process your bug report appropriately, we need you to provide the following additional information:
* ScummVM version (scummvm -v) * Bug details, including instructions on reproducing it * Language of game (English, German, etc) * Version of game (Talkie, Floppy...) * Platform and Compiler (Win32, Linux, MacOS, etc) * Attach a save game if possible * If this bug only occurred recently, please note the
last version without the bug, and the first version
including the bug. That way we can fix it quicker by
looking at the changes made.
This should only take you a little time but will make it much easier for us to
process your bug report in a way that satisfies both you and us.
Thank you for your support!