use assert etc. in Wine dlls? What is better?

Hi,

Maarten's wine-pulse contains several instances of assert()
I've been thinking about adding a bit of protection against
inconcsistencies within mmdevapi code too.

There are two variants that could be applicable depending on the situation:
/* this should not happen, but if we detect it early, we can work-around the situation */
if (a > limit) { ERR("%d > %d\n", a, limit); a = limit; }
or
assert(a <= limit, "whatever %d", a);

I'm not too fond of assert.  An assert in DSound was one of my first bug
fixes in Wine, years ago. It was not obvious why the app should not
continue running -- perhaps without sound.
OTOH, if an assert is violated, as a programmer, I'd like to hear about it.

I found little on that topic in Wine. In 2002, Alexandre Julliard wrote:
http://www.winehq.org/pipermail/wine-devel/2002-October/009766.html
> A better approach would be to leave assert() alone, and raise an
> exception on the SIGABRT signal.

What is current opinion on this topic?

Thank you,
 Jörg Höhle

[Michael Stefaniuc]

Hello Joerg,

On 03/13/2012 03:05 PM, Joerg-Cyril.Hoehle(a)t-systems.com wrote:
> Maarten's wine-pulse contains several instances of assert()
> I've been thinking about adding a bit of protection against
> inconcsistencies within mmdevapi code too.
>
> There are two variants that could be applicable depending on the situation:
> /* this should not happen, but if we detect it early, we can work-around the situation */
> if (a > limit) { ERR("%d > %d\n", a, limit); a = limit; }
> or
> assert(a <= limit, "whatever %d", a);
>
> I'm not too fond of assert.  An assert in DSound was one of my first bug
> fixes in Wine, years ago. It was not obvious why the app should not
> continue running -- perhaps without sound.
> OTOH, if an assert is violated, as a programmer, I'd like to hear about it.
>
> I found little on that topic in Wine. In 2002, Alexandre Julliard wrote:
> http://www.winehq.org/pipermail/wine-devel/2002-October/009766.html
> > A better approach would be to leave assert() alone, and raise an
> > exception on the SIGABRT signal.
>
> What is current opinion on this topic?
> there are quite a few assert calls used in the dlls:
> git grep -w assert dlls | grep -v "include" | wc -l
> 1509

But of course the correct answer when to add an assert is as always "It
depends" ;)
- If some internal state got corrupt and the app has no way to recover
but crash later on it is better to assert as early as possible as that
simplifies the debugging.
- If the app has a chance to deal with the error then returning an error
is the better approach. Add an ERR() or WARN() to that so we at least
can see the issue.

bye
	michael

[Maarten Lankhorst]

Hey,

Op 13-03-12 15:05, Joerg-Cyril.Hoehle(a)t-systems.com schreef:
> Hi,
>
> Maarten's wine-pulse contains several instances of assert()
> I've been thinking about adding a bit of protection against
> inconcsistencies within mmdevapi code too.
>
> There are two variants that could be applicable depending on the situation:
> /* this should not happen, but if we detect it early, we can work-around the situation */
> if (a > limit) { ERR("%d > %d\n", a, limit); a = limit; }
> or
> assert(a <= limit, "whatever %d", a);
>
> I'm not too fond of assert.  An assert in DSound was one of my first bug
> fixes in Wine, years ago. It was not obvious why the app should not
> continue running -- perhaps without sound.
> OTOH, if an assert is violated, as a programmer, I'd like to hear about it.
>
> I found little on that topic in Wine. In 2002, Alexandre Julliard wrote:
> http://www.winehq.org/pipermail/wine-devel/2002-October/009766.html
> The asserts I use are for internal consistency errors and shouldn't ever
> trigger. If they do trigger something has seriously gone wrong. For
> example This->pad <= This->bufsize_bytes should always hold true,
> and for render in the write callback, This->pad <= oldpad should hold
> true, since pulseaudio can only decrease padding. I don't even know
> how  to recover from this, since applications will bug if padding
> somehow increases since it's behavior that should never occur.
> None of the asserts should ever be triggered by misbehaving
> applications, unless there's a bug in pulseaudio.

~Maarten

[Frédéric Delanoy]

On Tue, Mar 13, 2012 at 15:05,  <Joerg-Cyril.Hoehle(a)t-systems.com> wrote:
> Hi,
>
> Maarten's wine-pulse contains several instances of assert()
> ...
> What is current opinion on this topic?

IMO asserts are for (fatal) errors that should never happen, that puts
a program into an inconsistent state.
Consider them as basic invariants of a program, indicative of some
serious bug, so better report them as early as possible, to prevent
seeing weird issues later where the source of the problem can be very
hard to detect/debug.

ERRs are for (possibly non-recoverable, but not necessarily) errors,
with a lower importance, i.e. they don't indicate a serious logic flaw
in the "core" of the code.

Just my 2 ¢.

Frédéric