Opened 16 years ago

Closed 16 years ago

Last modified 11 months ago

#1241 closed defect

ALL: Crash while exiting

Reported by: SF/arbonline Owned by: SF/jamieson630
Priority: normal Component: --Other--
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 fingolfin, 16 years ago

Status: newpending
Summary: Crash while exitingALL: Crash while exiting

comment:2 by fingolfin, 16 years ago

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!

comment:3 by SF/arbonline, 16 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 SF/arbonline, 16 years ago

Status: pendingnew

comment:5 by SF/jamieson630, 16 years ago

Owner: set to SF/jamieson630
Status: newpending

comment:6 by SF/jamieson630, 16 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 eriktorbjorn, 16 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 eriktorbjorn, 16 years ago

Status: pendingnew

comment:9 by SF/jamieson630, 16 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 SF/jamieson630, 16 years ago

Status: newpending

comment:11 by SF/arbonline, 16 years ago

Status: pendingnew

comment:12 by SF/arbonline, 16 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 SF/jamieson630, 16 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 SF/jamieson630, 16 years ago

Resolution: worksforme
Status: newclosed

comment:15 by fingolfin, 16 years ago

Resolution: worksforme
Status: closednew

comment:16 by fingolfin, 16 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 SF/jamieson630, 16 years ago

Status: newclosed

comment:18 by SF/jamieson630, 16 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 fingolfin, 16 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 digitall, 11 months ago

Component: --Unset----Other--
Note: See TracTickets for help on using tickets.