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.lu...@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);
_______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice