Re: [Libreoffice] OSL_ASSERT - remove backtrace or make abort

On Wednesday 20 of April 2011, Thorsten Behrens wrote:
> Lubos Lunak wrote:
> >  I'd like to remove the backtrace printing from OSL_ASSERT and friends,
> > or, even better and if possible, make these functions work properly, i.e.
> > abort on failure (I'm not really holding my breath on the second one, but
> > refusing that one will at least support the first one).

> Yes - first of all, there's SAL_DIAGNOSE_ABORT, to optionally enable
> your desired behaviour.

 I know, but that's useless in practice. If the usual approach is to treat 
OSL_ASSERT as non-fatal, then nobody will enable SAL_DIAGNOSE_ABORT, as that 
wouldn't get them very far. It makes more sense the other way around, the 
default being fatal and something like SAL_DIAGNOSE_DONT_ABORT allowing for 
an override in special cases.

> Then, there's sal-disable-backtrace.diff, 
> which I can happily merge - just set DISABLE_SAL_BACKTRACE then.

 Again, it's more about the default being wrong, as in the usual case the 
current default is about spamming the output with almost useless backtraces. 
Ok to use the attached patch instead?

> Regarding the problem itself, it's festering since many years, and
> not easily reconcilable - in the sal/uno area, assertions *are*
> serious, and should lead to aborts. Especially in the
> application/filter area, though, those were indeed often used in a
> "um, not sure, looks fishy here, let's do something"-kinda way.
>
> Generally, cleaning that up (and converting the mis-used assertions
> I mentioned into some warning) would be greatly appreciated.
> Something for past-3-4 and a feature branch?

 One thing worth noting is that the current behavior appears to be 
intentional: OSL_ASSERT stuff is documented and it says 
e.g. "OSL_ASSERT(cond) If cond is false, reports an error.", which I don't 
read as "and aborts". However, given how confusing the usage currently is in 
same places, I'd expect this not to be a bigger problem than some other way 
of cleaning this up.

 So, as the plan to handle this I'd suggest that we change the docs to say 
that these are now fatal and that non-fatal checks should use newly 
introduced OSL_WARNING(). Non-fatal OSL_ASSERT would however still be the 
default for the time being, unless let's say OSL_DIAGNOSE_FATAL is set. That 
would allow for transition where just a part of code would 
get -DOSL_DIAGNOSE_FATAL=1 in compiler flags and code could be slowly 
converted, similarly like some people now do with -Werror. It would be a bit 
harder to catch errors given that it's runtime (so I don't know if it would 
be suitable for EasyHacks), but there would be $SAL_DIAGNOSE_DONT_ABORT for 
such cases if one would run into something and wouldn't have time to fix it. 
It probably would be even non-intrusive enough to be doable directly in 
master, without a branch.

 Does that sound ok?

> IIRC OOo had some plans 
> to make at least smoketest completely 'assertion'-free.

 Given the recent news from Oracle, it's a question if we can expect anything 
from there. On the other side, if that's the case, it at least avoids the 
problem of OOo introducing new code using OSL_ASSERT as non-fatal.

-- 
 Lubos Lunak
 l.lunak@suse.cz

diff --git a/sal/osl/unx/diagnose.c b/sal/osl/unx/diagnose.c
index 30d15ad..9dae240 100644
--- a/sal/osl/unx/diagnose.c
+++ b/sal/osl/unx/diagnose.c
@@ -256,7 +256,9 @@ sal_Bool SAL_CALL osl_assertFailedLine (
     OSL_DIAGNOSE_OUTPUTMESSAGE(f, szMessage);
 
     /* output backtrace */
-    osl_diagnose_backtrace_Impl(f);
+    char const * envBacktrace = getenv( "SAL_DIAGNOSE_BACKTRACE" );
+    if( envBacktrace != NULL && *envBacktrace != '\0' );
+        osl_diagnose_backtrace_Impl(f);
 
     /* release lock and leave */
     pthread_mutex_unlock(&g_mutex);