Assertions and Logging

classic Classic list List threaded Threaded
51 messages Options
Next » 123
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Assertions and Logging

Hi all,

The current situation with assertions and logging of "interesting
events" in the LO code base is not very satisfying.

For one, there are two sets of functionality that cater for the same
needs.  One is osl/diagnose.h, the other is tools/debug.hxx.

For another, the distinction between true assertions (that assert
invariants of the program code, and that indicate programming errors
when not met) and the warning about unusual program states (that still
need to be handled properly by the code, like the occurrence of
malformed input) is not clear-cut with the existing macros.  Some code
is careful to use OSL_ASSERT only for true assertions and OSL_TRACE for
warnings about unusual program states, while other cdoee uses OSL_ASSERT
for both cases.

The downside of that mixture is that the useful debugging technique of
aborting upon detection of a violated invariant is not available.---If
you make OSL_ASSERT (and OSL_ENSURE, OSL_FAIL, etc.) abort, it will
abort far too often for mundane warnings for it to be useful.

And for a third, the combinatorial space of --enable-debug,
--enable-dbgutil, OSL_DEBUG_LEVEL, DBG_UTIL, etc. is rather confusing.
While --enable-debug (prepare the build for easy debugging, by letting
the compiler emit extra debug information and disable optimizations,
etc.) and --enable-dbgutil (enable additional checks in the code base)
ought to be rather orthogonal, in some sense they ultimately converge on
the same OSL_DEBUG_LEVEL macro, trying to control disparate needs with a
mere three values, 0, 1, 2.  (--enable-debug and --enable-dbgutil both
set OSL_DEBUG_LEVEL to 1; setting it to 2 requires passing a dbglevel=2
switch to make; additionally, --enable-dbgutil defines the DBG_UTIL
macro.)  Also, confusion keeps arising as to whether enabling various
debugging mechanisms may render the code for which they have been
enabled incompatible (i.e., whether they have to be enabled for the
complete build, or can be enabled selectively).  The general idea is
that --enable-debug does not cause incompatibility, while
--enable-dbgutil potentially does (and even uses another $INPATH,
removing the ".pro" extension).


That being the status quo, an analysis of what would actually be useful
to have brings us to the following three points:

1  There is demand for "true" assertions (that identify errors in
program logic and abort).  I suggest to use standard <assert.h> for
them.  It is available everywhere (in every module, in C as well as
C++), and no home-brown solution is needed.  It might be considered a
disadvantage that assert does not allow to supply an additional error
message.  However, as a failed assert promptly aborts program execution
with an appropriate message (supplied by the compiler, containing file
and line information), the missing message is not much of a
nuisance---you will need to look at the specified program location right
away anyway, to debug the problem.  (On Unix-like OSs, assert generally
outputs to stderr; for Windows GUI applications, it outputs to a message
box and allows attaching a debugger, see the MSDN documentation for
"assert" at
<http://msdn.microsoft.com/en-us/library/9sb57dw4%28v=vs.71%29.aspx>.)

Whether assertions are enabled (and abort when not met) is ontrolled via
the standard NDEBUG macro.  It is already taken care of in our build
environment, being left undefined (i.e., assertions are enabled) for
--enable-dbgutil and for --enable-debug.  (In the old build system, it
is only left undefined for --enable-dbgutil, and always defined---thus
disabling assertions---otherwise.)  As enabling assertions should not
affect compatibility, enabling them for booth --enable-dbgutil and
--enable-debug appears appropriate.  (But we can leave the behaviour of
the old build system as it is, given that it is going away, anyway.)

2  There is also demand for logging of "interesting events" during code
execution.  For this, I propose new functionality below.  Whether or not
logging is enabled should not affect compatibility.

The rationale for replacing the current mechanisms for both (1) and (2)
with something else each, is to allow for a smooth transition.  Existing
occurrences of OSL_ASSERT etc. can be inspected individually, replacing
them either with assert (see above) or SAL_WARN (see below).  If we
would instead keep OSL_ASSERT for one of those two cases, it would be
unclear for an occurrences of OSL_ASSERT in the code whether it has
already been visited and reclassified or not.

3  Furthermore, there is sometimes demand for additional, debug-only
code.  In general, that would be safety-check additions that are
considered too expensive to be included in production builds (e.g., code
that iterates over a data structure to check invariants, or additional,
redundant fields within data structures).  Enabling such additional code
potentially affects compatibility.

Such additional code is currently controlled via OSL_DEBUG_LEVEL et al.
  OSL_DEBUG_LEVEL==1 is generally used for additions that do not affect
compatibility (as it is enabled by both --enable-debug and
--enable-dbgutil).  OSL_DEBUG_LEVEL==2, DBG_UTIL (defined upon
--enable-dbguitl) and privately invented defines in certain parts of the
code (to be set manually by knowledgeable developers) are used for
additions that affect compatibility or that are considered too specific
for general inclusion.  Either because they are too expensive even for
every --enable-dbgutil build, or because they produce excessive log
information (for which case the below new log functionality offers a
better solution).

This can probably be reduced to three cases:

#if OSL_DEBUG_LEVEL != 0  for additional code that does not cause
incompatibilities (if there is still demand for such code; the new log
functionality will remove the need for such code in many cases).  This
effectively reduces OSL_DEBUG_LEVEL to a binary switch (so the make
dbglevel=x feature can be removed; and OSL_DEBUG_LEVEL could potentially
be renamed---but that would probably not be worth it).

#if defined DBG_UTIL  for additional code that causes incompatibilities.

#if defined MY_SPECIAL_DEBUG  (replaced with actual defines, varying
across the different code modules) for those special cases where always
enabling the additional code is deemed to expensive in general.
(However, for those special cases where the additional code produces
excess log information, see below.)


Which brings us to the new log functionality.

Attached is a patch against current (towards LO 3.5) master (actually
two patches, an extra one for binfilter).  It introduces sal/log.h and
makes osl/diagnose.h and tools/debug.h divert to it internally.  It also
replaces a few example occurrences of the old osl/diagnoes.h and
tools/debug.h macros with their new counterparts (and I encourage you to
take a look at those replacements, to get a feel for what is possible
with the new functionality).

For easy reference, I copy the relevant documentation from sal/log.h here:

>     SAL_INFO(char const * area, char const * format, ...),
>     SAL_INFO_IF(bool condition, char const * area, char const * format, ...),
>     SAL_WARN(char const * area, char const * format, ...), and
>     SAL_WARN_IF(bool condition, char const * area, char const * format, ...)
>     produce an info resp. warning log entry with a printf-style message.  The
>     given format argument and any following arguments must be so that that
>     sequence of arguments would be appropriate for a call to printf.
>
>     SAL_INFO_S(char const * area, expr),
>     SAL_INFO_IF_S(bool condition, char const * area, expr),
>     SAL_WARN_S(char const * area, expr), and
>     SAL_WARN_IF_S(bool condition, char const * area, expr) produce an info resp.
>     warning log entry with a message produced by piping items into a C++
>     std::ostringstream (and are only available in C++).  The given expr must be
>     so that the full expression "stream << expr" is valid, where stream is a
>     variable of type std::ostringstream.
>
>       SAL_INFO_S("foo", "string " << s << " of length " << n)
>
>     would be an example of such a call; if the given s is of type rtl::OUString,
>
>       #include "rtl/oustringostreaminserter.hxx"
>
>     would make sure that an appropriate operator << is available.
>
>     For the _IF variants, log output is only generated if the given condition is
>     true (in addition to the other conditions that have to be met).
>
>     For all these macros, the given area argument must be non-null and must
>     match the regular expression
>
>       <area> ::= <subarea>("."<subarea>)*
>
>     with
>
>       <subarea> ::= [0-9a-z]+
>
>     Whether these macros generate any log output is controlled in a two-stage
>     process.
>
>     First, at compile time the macro SAL_LOG_LEVEL controls whether these macros
>     expand to actual code, or to no-ops.  SAL_LOG_LEVEL must expand to an
>     integral value 0, 1, or 2.
>
>     If SAL_LOG_LEVEL is 0, neither the INFO nor the WARN macros produce code.
>     If SAL_LOG_LEVEL is 1, only the WARN macros produce code.  If SAL_LOG_LEVEL
>     is 2, both the INFO and the WARN macros produce code.
>
>     Second, at runtime the environment variable SAL_LOG further limits which
>     macro calls actually generate log output.  The environment varialbe SAL_LOG
>     must either be unset or must match the regular expression
>
>       <env> ::= <switch>*
>
>     with
>
>       <switch> ::= <sense><level><area>?
>       <sense> ::= "+"|"-"
>       <level> ::= "INFO"|"WARN"
>
>     If the environment variable is unset, "+WARN" is used instead (which results
>     in all warnings being output but no infos).  If the given value does not
>     match the regular expression, "+INFO+WARN" is used instead (which in turn
>     results in everything being output).
>
>     A given macro call's level (INFO or WARN) and area is matched against the
>     given switches as follows:  Only those switches for which the level matches
>     the given level and for which the area is a prefix (including both empty and
>     full prefixes) of the given area are considered.  Log output is generated if
>     and only if among the longest such switches (if any), there is at least one
>     that has a sense of "+".  (That is, if both +WARN.foo and -WARN.foo are
>     present, +WARN.foo wins.)
>
>     For example, if SAL_LOG is "+INFO-INFO.foo+INFO.foo.bar", then calls like
>     SAL_INFO("foo.bar", ...), SAL_INFO("foo.bar.baz", ...), or
>     SAL_INFO("other", ...) generate output, while calls like
>     SAL_INFO("foo", ...) or SAL_INFO("foo.barzzz", ...) do not.
>
>     The generated log output consists of the given level ("info" or "warn"), the
>     given area, the process ID, the thread ID, the source file, and the source
>     line number, each followed by a colon, followed by a space, the given
>     message, and a newline.  The given message should contain no vertical
>     formatting characters and no null characters.  The precise format of the log
>     output is subject to change.  The log output is printed to stderr.
Some further points:

- Whether or not both SAL_INFO and SAL_WARN produce actual code is
currently effectively controlled via OSL_DEBUG_LEVEL:  With either
--enable-dbgutil or --enable-debug, the full log functionality is
enabled.  Whether or not a more fine-grained control (producing code for
only SAL_WARN) is necessary needs to be seen.  (If not, SAL_LOG_LEVEL
can be folded into OSL_DEBUG_LEVEL.)  Having SAL_INFO compiled into each
--enable-dbgutil build makes it possible to selectively enable
interesting log information via SAL_LOG=+INFO.foo without having to
recompile code.

- Assigning area codes to the macro invocations, I have mostly stuck to
single-segment area codes that match the names of the code modules
("sal", "binfilter", "sfx2").  For the replacements within
osl/diangose.h and tools/debug.hxx (that will in turn be called from
various places, so it would not make much sense to attribute them to
"sal" or "tools", respectively) I used "legacy.osl" and "legacy.tools",
respectively.  For a few cases where old code was OSL_DEBUG_LEVEL>=2
conditional, I used two-segment area codes "canvas.level2", "jfw.level2"
(and "jfw.level1"), "sw.level2".

- Replacing all existing calls to deprecated osl/diagnose.h and
tools/debug.hxx macros will need to happen over time (an easy hack).

- There is further functionality in tools/debug.hxx (DBG_MEMTEST,
DBG_CTOR, etc.) that has not yet been addressed.

- The recently introduced OSL_FORMAT is superseded by SAL_STREAM (which
uses C++-stream-style composition instead of C-printf-format-style).
OSL_FORMAT and all its uses have been removed.

- The implementation is somewhat careful to produce as little code as
possible at the call-site of the new macros, and to keep the code path
for suppressed logs as short as possible.  However, the C++-stream-style
macros will potentially always be more expensive than the
C-printf-format-style ones.  But the former are more useful, so I would
encourage you to nevertheless use them where appropriate.

- unotest/oustringostreaminserter.hxx is moved to
rtl/oustringostreaminserter.hxx (and duplications of it below unotest
are removed); it is often needed in combination with sal/log.h.

- In a few places, the patches replace occurrences of tools String with
rtl::OUString, so that streaming works.  Otherwise, it is necessary to
wrap the tools String in an OUString ctor.

- sal/log.h contains a TODO to enable a GCC __attribute__((format)) on
the C funtion underneath the C-printf-format-style macros.  Enabling it
would produce tons of -Werror=format from old OSL_TRACE uses.  These
need to be cleaned up first (an easy hack).

One open question is which set of macros (the C-printf-format-style ones
or the C++-stream-style ones) to give the "natural" names without
additional suffix.  For now, I reserve the natural names for the C-style
ones, as they are more universal (not only available in C++) and produce
smaller call-side code, especially for the simple, common case where the
message consists of just a string literal, as in SAL_INFO("foo",
"message").  However, reserving the natural names for the C++-style
macros (and giving the other ones an _F suffix, say) would be justified
by their better usability (and thus more frequent usage) and the fact
that they avoid the pitfall of having to escape any literal "%" within
the format string of a C-style macro usage---although enabling GCC's
__attribute__((format)) check would also help detect misuses like
SAL_INFO("foo", "100%").

If there is no objection, I will push the patches to master next week.
Due to removing OSL_FORMAT and osl_detail_formatString again, I would
prefer to get this still into LO 3.5.

Stephan

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

log.patch (140K) Download Attachment
log-binfilter.patch (14K) Download Attachment
Caolán McNamara Caolán McNamara
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

On Fri, 2011-11-18 at 15:25 +0100, Stephan Bergmann wrote:

Alright, so we end up with
use assert if you want an assert
SAL_WARN if you want to warn about something odd, but which isn't
necessarily definitely wrong
SAL_INFO for verbose logging

Practical question though, is on windows where does the output go ?

Should we write off all the DBG_ASSERTs as hopeless to unwind and mass
convert them all to SAL_WARNs ?

How do we feel about code that does e.g.

assert(pFoo);
if (!pFoo)
    throw catchAbleFoo("wtf");

i.e. do we have a philosophical problem with gracefully/semi-gracefully
handing should-be impossible cases ?

C.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

On 11/21/2011 01:30 PM, Caolán McNamara wrote:
> On Fri, 2011-11-18 at 15:25 +0100, Stephan Bergmann wrote:
>
> Alright, so we end up with
> use assert if you want an assert
> SAL_WARN if you want to warn about something odd, but which isn't
> necessarily definitely wrong
> SAL_INFO for verbose logging
>
> Practical question though, is on windows where does the output go ?

SAL_INFO/WARN just go to stderr for now.  What should work to see them
even for a gui soffice.exe is to add something like 2>log.txt to the
command line.  Not sure if there is enough of a Windows developer base
that demands a more sophisticated solution?  (One could extend the
SAL_LOG environment variable, so that a trailing ">file" part would
append the data to a given file, for example.  Or see to hook up Window'
OutputDebugString.)

> Should we write off all the DBG_ASSERTs as hopeless to unwind and mass
> convert them all to SAL_WARNs ?

I would initially file an easy hack to do the conversion.  If that does
not lead to clean-up in a reasonable time-frame, I would suggest to
mass-convert the remainder to SAL_WARN.

> How do we feel about code that does e.g.
>
> assert(pFoo);
> if (!pFoo)
>      throw catchAbleFoo("wtf");
>
> i.e. do we have a philosophical problem with gracefully/semi-gracefully
> handing should-be impossible cases ?

I think that's a perversion, and should be avoided.  It the author could
not convince himself that !pFoo is not impossible (modulo bugs), then he
should use OSL_WARN instead.  If however he *is* convinced that !pFoo is
impossible absent any bugs, but argues that if there *are* bugs, the
added if statement adds some sort of safety, I would counter-argue that
this alleged safety net pointlessly increases complexity and is probably
full of holes and inconsistencies anyway (as it is likely an extremely
untested code path).

Such defensive programming IMO would need to be architected into the
code from the beginning.  It makes little sense to add such things
locally just here and there.  And I'm not even convinced LO is an
application for which an elaborate defensive programming architecture
would be justified.  If there is a bug, crash early.  (One thing we
could IMO improve though, is to not rely on trying to save open
documents from within a signal handler, but instead rely on frequent
auto-save and roll back to the last saved version after a crash.)

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Terrence Enger Terrence Enger
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

On Mon, 2011-11-21 at 14:30 +0100, Stephan Bergmann wrote:
>                                                       (One thing we
> could IMO improve though, is to not rely on trying to save open
> documents from within a signal handler, but instead rely on frequent
> auto-save and roll back to the last saved version after a crash.)

Not disagreeing, but this would increase the fraction of the
time during which Writer, at least, ignores keystrokes.  If
you are looking at the screen when this happens, you just
wait for the autosave to finish and then carry on; if you
are looking at something else, you are faced later--for some
walue of "later"--with the challenge of trying to
reconstruct what you were thinking.

Terry.


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

On 11/21/2011 04:14 PM, Terrence Enger wrote:

> On Mon, 2011-11-21 at 14:30 +0100, Stephan Bergmann wrote:
>>                                                        (One thing we
>> could IMO improve though, is to not rely on trying to save open
>> documents from within a signal handler, but instead rely on frequent
>> auto-save and roll back to the last saved version after a crash.)
>
> Not disagreeing, but this would increase the fraction of the
> time during which Writer, at least, ignores keystrokes.  If
> you are looking at the screen when this happens, you just
> wait for the autosave to finish and then carry on; if you
> are looking at something else, you are faced later--for some
> walue of "later"--with the challenge of trying to
> reconstruct what you were thinking.

True.  So an improvement probably better left for the hypothetical
future where we have really fast save and/or save in the background.

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Caolán McNamara Caolán McNamara
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg
On Mon, 2011-11-21 at 14:30 +0100, Stephan Bergmann wrote:
> On 11/21/2011 01:30 PM, Caolán McNamara wrote:
> > Practical question though, is on windows where does the output go ?
>
> SAL_INFO/WARN just go to stderr for now.  What should work to see them
> even for a gui soffice.exe is to add something like 2>log.txt to the
> command line.  

oh, does that work ? I was labouring under the misunderstanding that we
closed those streams under windows, or something of that nature. I'm a
complete windows weenie.

> >
> > assert(pFoo);
> > if (!pFoo)
> >      throw catchAbleFoo("wtf");
> >
> > i.e. do we have a philosophical problem with gracefully/semi-gracefully
> > handing should-be impossible cases ?
>
> I think that's a perversion, and should be avoided.  It the author could
> not convince himself that !pFoo is not impossible (modulo bugs), then he
> should use OSL_WARN instead.  If however he *is* convinced that !pFoo is
> impossible absent any bugs, but argues that if there *are* bugs, the
> added if statement adds some sort of safety,

I have no strong feelings either way, but might as well agree now while
we can. So the plan is that asserts are for 100% can never happen
things. So that would suggest that anything which might fail for
external reasons is not a candidate for assert.

oslModule hModule= osl_loadModule( foo );
assert(hModule)
if (!hModule)
    throw bar;

is wrong, because foo might not exist if some member of the lunatic
fringe deleted some .sos out of his install. Or more fairly, his distro
tried to split up packages into subpackages and mis-categorized one of
them.

In which case, we should use SAL_WARN to indicate its an unlikely and
suspicious event. So do we then consider SAL_WARNs as failures from the
perspective of e.g. the smoketest where we can argue reasonably that
we're in a controlled environment and nothing unusual should occur ?

C.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg

On Mon, 2011-11-21 at 16:23 +0100, Stephan Bergmann wrote:
> True.  So an improvement probably better left for the hypothetical
> future where we have really fast save and/or save in the background.

        Of course; in a world of interactive co-editing; we should really
stream everything people edit to a journal file using that serialisation
& then replay it when we re-start ;-) [ and then kill autosave
altogether ].

        But that of course requires rather more work.

> (One thing we could IMO improve though, is to not rely on trying to
> save open documents from within a signal handler, but instead rely on
> frequent auto-save and roll back to the last saved version after a
> crash.)

        Until then, I think we need to stick with the signal handler, sadly, it
is truly ugly, and I assume it can deadlock too if the signal happens at
certain places wrt. holding mutex', remarkably few posix calls are
signal-safe.

        HTH,

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

On 11/21/2011 04:50 PM, Michael Meeks wrote:
> Until then, I think we need to stick with the signal handler, sadly, it
> is truly ugly, and I assume it can deadlock too if the signal happens at
> certain places wrt. holding mutex', remarkably few posix calls are
> signal-safe.

One indeed occasionally observes "hanging crashes" or "crashes within
crashes" caused by the illegal activities we start off the signal handler.

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by Caolán McNamara
On 11/21/2011 04:42 PM, Caolán McNamara wrote:

> On Mon, 2011-11-21 at 14:30 +0100, Stephan Bergmann wrote:
>> On 11/21/2011 01:30 PM, Caolán McNamara wrote:
>>> Practical question though, is on windows where does the output go ?
>>
>> SAL_INFO/WARN just go to stderr for now.  What should work to see them
>> even for a gui soffice.exe is to add something like 2>log.txt to the
>> command line.
>
> oh, does that work ? I was labouring under the misunderstanding that we
> closed those streams under windows, or something of that nature. I'm a
> complete windows weenie.

I remember it at least used to work even with 4NT back in those days
(the supported syntax there was the csh >&, IIRC).  No idea whether we
started to close stderr for some reason since then, though.

>>> assert(pFoo);
>>> if (!pFoo)
>>>       throw catchAbleFoo("wtf");
>>>
>>> i.e. do we have a philosophical problem with gracefully/semi-gracefully
>>> handing should-be impossible cases ?
>>
>> I think that's a perversion, and should be avoided.  It the author could
>> not convince himself that !pFoo is not impossible (modulo bugs), then he
>> should use OSL_WARN instead.  If however he *is* convinced that !pFoo is
>> impossible absent any bugs, but argues that if there *are* bugs, the
>> added if statement adds some sort of safety,
>
> I have no strong feelings either way, but might as well agree now while
> we can. So the plan is that asserts are for 100% can never happen
> things. So that would suggest that anything which might fail for
> external reasons is not a candidate for assert.
>
> oslModule hModule= osl_loadModule( foo );
> assert(hModule)
> if (!hModule)
>      throw bar;
>
> is wrong, because foo might not exist if some member of the lunatic
> fringe deleted some .sos out of his install. Or more fairly, his distro
> tried to split up packages into subpackages and mis-categorized one of
> them.

Yes, that's my model, too.  Even if missing/corrupted files that belong
to the installation are somewhat more on the scale towards "cannot
happen" than, say, trying to read malformed .odt files, they still
represent external errors, not logic programming errors.

It would be good if such "impossible to proceed" situations would lead
to uncaught exceptions (which is IMO acceptable in case of a fucked up
installation) or clear error messages from within LO ("this
functionality is unavailable (detailed error message: ...)"), instead of
LO silently doing nothing (as is so often the case today, e.g., when you
choose a menu item that triggers functionality that requires Java, and
you don't have a JVM).

> In which case, we should use SAL_WARN to indicate its an unlikely and
> suspicious event. So do we then consider SAL_WARNs as failures from the
> perspective of e.g. the smoketest where we can argue reasonably that
> we're in a controlled environment and nothing unusual should occur ?

I'm somewhat undecided here.  The smoketest used to fail on OSL_ASSERTs
(at least partly motivated by the assumption that OSL_ASSERTs represent
true assertions; even if that assumption was false, the OSL_ASSERTs that
did fire during smoketest were generally either true assertions that got
fixed, or of informative nature and degraded to OSL_TRACE, IIRC).  I
think I effectively removed that with my patch (it still fails for true
asserts that abort now, of course), but intended to revisit that.  On
the one hand, your rationale is probably true that SAL_WARNs probably
always indicate severe enough problems that they would not normally fire
in the controlled environment of smoketest.  So, from a practical
standpoint, failing smoketest on failed SAL_WARNs would be right.  On
the other hand, there might be SAL_WARNs that legitimately fire during
smoketest (a trivial example would be if we purposefully tested illegal
input during smoketest), so from a theoretical standpoint failing
smoketest on failed SAL_WARNs would be wrong.  But I think I'll stick
with the practical standpoint for now (if only since its easier to
eventually change from a failing smoketest to one that ignores SAL_WARNs
than vice versa).

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Kevin Hunter Kevin Hunter
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

At 11:53am -0500 Mon, 21 Nov 2011, Stephan Bergmann wrote:
> It would be good if such "impossible to proceed" situations would
> lead to uncaught exceptions [...] or clear error messages from within
> LO ("this functionality is unavailable (detailed error message:
> ...)"), instead of LO silently doing nothing (as is so often the case
> today, e.g., when you choose a menu item that triggers functionality
> that requires Java, and you don't have a JVM).

I'm not sure if that was a conclusion to this thread, but are we
documenting this kind of decision in a concise format (and hopefully
single page) on the wiki?  After a few minutes of looking, all I see is
this page:

http://wiki.documentfoundation.org/Development#General_Programming_Guidelines

For us non-core folks who aren't able to follow every message, and new
devs in general, having a page of "LO coding style and various examples
of what (not) to do" would, at least for me, be very helpful.

Kevin
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg
Hi Stephan,

On Monday, 2011-11-21 17:53:42 +0100, Stephan Bergmann wrote:

> On 11/21/2011 04:42 PM, Caolán McNamara wrote:
> >On Mon, 2011-11-21 at 14:30 +0100, Stephan Bergmann wrote:
> >>SAL_INFO/WARN just go to stderr for now.  What should work to see them
> >>even for a gui soffice.exe is to add something like 2>log.txt to the
> >>command line.
> >
> >oh, does that work ? I was labouring under the misunderstanding that we
> >closed those streams under windows, or something of that nature. I'm a
> >complete windows weenie.
>
> I remember it at least used to work even with 4NT back in those days
> (the supported syntax there was the csh >&, IIRC).  No idea whether
> we started to close stderr for some reason since then, though.
From what I vaguely remember it is Windows that closes those streams for
GUI executables, at least if they point to a terminal, it might work if
redirected, don't know. In Windows builds there used to be
a solver/$INPATH/bin/guistd.com or some such that if copied to the path
of soffice.exe and renamed to soffice.com and invoked instead preserved
stdout/stderr streams. Maybe it's still built.

  Eike

--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

> From what I vaguely remember it is Windows that closes those streams for
> GUI executables,

Something like that, yes, although they are not "closed" in the sense
that they would first be open ("inherited" in the Unix sense); they
are never open in the first place in GUI executables, unless
explicitly redirected when creating the process (which the shell,
cmd.exe, does when you redirect input/out from/to a file/pipe). (HEre
I talk about the underlying Win32 file handles; the C library's file
descriptors, and the corresponding C++ streams probably *are* "open",
even if connected to invalid file handles.)

> In Windows builds there used to be
> a solver/$INPATH/bin/guistd.com or some such that if copied to the path
> of soffice.exe and renamed to soffice.com and invoked instead preserved
> stdout/stderr streams. Maybe it's still built.

Not for soffice, there we just have soffice.exe and soffice.bin. But
for unopkg we deliver three different exectuables: unopkg.com,
unopkg.exe and unopkg.bin, which start each others in that order, if I
understand correctly.

Now, if I understand the purpose of the outermost wrapper, the
guistdio == unopkg.com thing, that is probably not needed any more on
modern Windowses (XP or later) as it is possible for a process to
"attach" to the console (think controlling terminal in Unix terms) of
the parent process, and thus acquire the possibility to reopen its
stdin/out/err to the paren't process's console. Look for documentation
for AttachConsole(ATTACH_PARENT_PROCESS).

--tml
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Norbert Thiebaud Norbert Thiebaud
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by Caolán McNamara
On Mon, Nov 21, 2011 at 6:30 AM, Caolán McNamara <[hidden email]> wrote:
> On Fri, 2011-11-18 at 15:25 +0100, Stephan Bergmann wrote:
>
>
> Practical question though, is on windows where does the output go ?

Actually I have a similar scheme for my own stuff, and I send the
output to <program>_<pid>.log in the current working directory. if the
current working directory is not writable, I send it to that same file
but in $TMP. For libreoffice an alternative could be the ~/.lo
directory or whatever the equivalent is on windows...

Note that using stderr/stdout may be a problem as that could interfere
with other stuff. on windows there is not always a Console associated
with the execution... and our codebase is also used for other, more
batch oriented piece of code... messing with stderr/stdout may
actually be a pain.

I also have mechnism so that if no message is written, the log file is
actually cleaned-up on termination. iow some kind a mechanism to avoid
these log file to polute too much (maybe a kind of 'clean on startup'
to get rid of old lingering log files)

The biggest issue on windows, is 'flush' (well that is a problem in C
because windows does not do signal handling... but with exception on
C++ maybe there is a way around that)

Norbert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Norbert Thiebaud Norbert Thiebaud
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg
On Fri, Nov 18, 2011 at 8:25 AM, Stephan Bergmann <[hidden email]> wrote:
>>
>>    Second, at runtime the environment variable SAL_LOG further limits
>> which
>>    macro calls actually generate log output.  The environment varialbe
>> SAL_LOG
>>    must either be unset or must match the regular expression

The run tine flexibility is a great feature, but the proposed
implementation is scary perf-wise

you don't want to re-parse the log-config at every log message.

You need to parse it once and for all... and not use string but enum
to identify the module/category you want to trace.

as a one-time steup cost you parse you env variable and initialize a
static array, which for each module/category contain the max
acceptable level of log
(0 = no log, 1=error, 2=warn, 3=info (4=debug?)

the test of log-ability should  be a simple if( array[module] >=
level) and the macro writtin so that the no-trace path is as cheap as
possible.

To illustrate (this is for pure C where each 'module' actually have a
init_module function to dynamically register themselves to the tracing
system... that may not work nicely in c++ (ironically), but the
general gist is the same:

(excerpt)

header:

#define _msgs_out(rc) { msgs_trace_intern("%s <-- 0x%x", __func__, rc);};
#define _msgs_cond(lvl) if((lvl <= MSGS_BUILD_TRACE_LEVEL) && (lvl <=
MSGS_CONTEXT->level))

#define msgs_major     _msgs_cond(LVL_MAJOR)     msgs_trace_intern
#define msgs_normal    _msgs_cond(LVL_NORMAL)    msgs_trace_intern
#define msgs_minor     _msgs_cond(LVL_MINOR)     msgs_trace_intern
#define msgs_detail    _msgs_cond(LVL_DETAIL)    msgs_trace_intern
#define msgs_verbose   _msgs_cond(LVL_VERBOSE)   msgs_trace_intern
#define msgs_debug     _msgs_cond(LVL_DEBUG)     msgs_trace_intern
#define msgs_logorrhea _msgs_cond(LVL_LOGORRHEA) msgs_trace_intern
#define msgs_uncond    msgs_trace_intern


static inline void msgs_trace_intern(const char format[], ...)
{
va_list list;

    va_start(list, format);
    msgs_do_trace(MSGS_CONTEXT, format, list);
    va_end(list);
}

here MSGS_CONTEXT could be &g_aModules_Context[module_id] , modifying
the api above, adding a module_id parm (in my case the module_id is
implicit based on the location of the trace)

Note: the lvl <= MSGS_BUILD_TRACE_LEVEL in _msgs_cond means that the
whole things is optimized out at compile if the test is false (both
part of the test are actually build time constant. the second part f
the test means thta the test cost just a couple of integer compare to
fail.

Note that I have an intermediary static inlinee msgs_trace_intern due
to the fact that I use a static variable MSGS_CONTEXT, static to each
'module'... in you case you do not need that, if you do a full
centralized pre-registration.

Note that you can use:

log(writer,....)

 and in the define of the log macro use module ## _LOG_ID to refer to
the enum value, and #module to have a pretty sting to print, so that
your log still show a human readable module name

for example, I use something like :

/* We need two macro here, to have a proper expansion of the 'module'
parameter */
#define CORE_DECLARE_MODULE(module) CORE_DECLARE_MODULE2(module)

#define CORE_DECLARE_MODULE2(module) \
static struct msgs_module_context g_context = { module ##  _MODULE_ID
, -1, 0, 0, NULL,  #module}

to define my module_level context

Norbert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Jonathan Aquilina Jonathan Aquilina
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

On 22/11/2011 06:35, Norbert Thiebaud wrote:

> On Fri, Nov 18, 2011 at 8:25 AM, Stephan Bergmann<[hidden email]>  wrote:
>>>     Second, at runtime the environment variable SAL_LOG further limits
>>> which
>>>     macro calls actually generate log output.  The environment varialbe
>>> SAL_LOG
>>>     must either be unset or must match the regular expression
> The run tine flexibility is a great feature, but the proposed
> implementation is scary perf-wise
>
> you don't want to re-parse the log-config at every log message.
>
> You need to parse it once and for all... and not use string but enum
> to identify the module/category you want to trace.
>
> as a one-time steup cost you parse you env variable and initialize a
> static array, which for each module/category contain the max
> acceptable level of log
> (0 = no log, 1=error, 2=warn, 3=info (4=debug?)

Wouldnt a linked list be better as we dont know how many of these error
messages might end up occuring for each during build time?

>
> the test of log-ability should  be a simple if( array[module]>=
> level) and the macro writtin so that the no-trace path is as cheap as
> possible.
>
> To illustrate (this is for pure C where each 'module' actually have a
> init_module function to dynamically register themselves to the tracing
> system... that may not work nicely in c++ (ironically), but the
> general gist is the same:
>
> (excerpt)
>
> header:
>
> #define _msgs_out(rc) { msgs_trace_intern("%s<-- 0x%x", __func__, rc);};
> #define _msgs_cond(lvl) if((lvl<= MSGS_BUILD_TRACE_LEVEL)&&  (lvl<=
> MSGS_CONTEXT->level))
>
> #define msgs_major     _msgs_cond(LVL_MAJOR)     msgs_trace_intern
> #define msgs_normal    _msgs_cond(LVL_NORMAL)    msgs_trace_intern
> #define msgs_minor     _msgs_cond(LVL_MINOR)     msgs_trace_intern
> #define msgs_detail    _msgs_cond(LVL_DETAIL)    msgs_trace_intern
> #define msgs_verbose   _msgs_cond(LVL_VERBOSE)   msgs_trace_intern
> #define msgs_debug     _msgs_cond(LVL_DEBUG)     msgs_trace_intern
> #define msgs_logorrhea _msgs_cond(LVL_LOGORRHEA) msgs_trace_intern
> #define msgs_uncond    msgs_trace_intern
>
>
> static inline void msgs_trace_intern(const char format[], ...)
> {
> va_list list;
>
>      va_start(list, format);
>      msgs_do_trace(MSGS_CONTEXT, format, list);
>      va_end(list);
> }
>
> here MSGS_CONTEXT could be&g_aModules_Context[module_id] , modifying
> the api above, adding a module_id parm (in my case the module_id is
> implicit based on the location of the trace)
>
> Note: the lvl<= MSGS_BUILD_TRACE_LEVEL in _msgs_cond means that the
> whole things is optimized out at compile if the test is false (both
> part of the test are actually build time constant. the second part f
> the test means thta the test cost just a couple of integer compare to
> fail.
>
> Note that I have an intermediary static inlinee msgs_trace_intern due
> to the fact that I use a static variable MSGS_CONTEXT, static to each
> 'module'... in you case you do not need that, if you do a full
> centralized pre-registration.
>
> Note that you can use:
>
> log(writer,....)
>
>   and in the define of the log macro use module ## _LOG_ID to refer to
> the enum value, and #module to have a pretty sting to print, so that
> your log still show a human readable module name
>
> for example, I use something like :
>
> /* We need two macro here, to have a proper expansion of the 'module'
> parameter */
> #define CORE_DECLARE_MODULE(module) CORE_DECLARE_MODULE2(module)
>
> #define CORE_DECLARE_MODULE2(module) \
> static struct msgs_module_context g_context = { module ##  _MODULE_ID
> , -1, 0, 0, NULL,  #module}
>
> to define my module_level context
>
> Norbert
> _______________________________________________
> LibreOffice mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/libreoffice

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by Norbert Thiebaud
On 11/22/2011 06:35 AM, Norbert Thiebaud wrote:
> The run tine flexibility is a great feature, but the proposed
> implementation is scary perf-wise

I kept the implementation simplistic on purpose.  First, remember that
logging (at least for now) is only enabled in debug/dbgutil builds, and
that the SAL_LOG environment variable will have a rather short value
most of the time (the default is just "+WARN").  Beware premature
optimization.

Second, static data is a problem, as is initialize-once data in a
multi-threaded world.  To properly initialize it you need
synchronization mechanisms, which are either platform specific or need
to use osl/mutex.h, which in turn is undesirable.  "Avoid the use of
other sal code in this file as much as possible, so that this code can
be called from other sal code without causing endless recursion."  I
already felt uneasy enough adding OSL_DETAIL_GETPID.

Third, free-form string area codes are indeed not optimal.  But I fear
typos here more than I fear performance problems.  And their advantage
is that they need zero centralized configuration.

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg
I pushed the proposed changes as
<http://cgit.freedesktop.org/libreoffice/core/commit/?id=70a6b9ffbd676a1384433a86205d2cd4f2d4f4b1>
and
<http://cgit.freedesktop.org/libreoffice/binfilter/commit/?id=d45d5ee81e3d3f1779774af3f400ce3f1aa6697d>.
  Any further refinements (where to send the log output, performance
considerations) can carry on from there.

I added a short summary as
<https://wiki.documentfoundation.org/Development#Assertions_and_Logging>; feel
free to expand.  I also filed easy hack
<https://bugs.freedesktop.org/show_bug.cgi?id=43157> "Clean up
OSL_ASSERT, DBG_ASSERT, etc."  As already discussed, if any uses of the
obsolete functionality still remains after a while, I would initiate a
mass-conversion OSL_ASSERT->SAL_WARN, OSL_TRACE->SAL_INFO, etc.

I made a few further changes to the proposed patches when I pushed them:

- DBG_ASSERT etc. from tools/debug.hxx still are only enabled depending
on DBG_UTIL.  Trying an --enable-debug --disable-dbgutil build, it broke
at many places that #ifdef DBG_UTIL some code that is later used within
a DBG_ASSERT (e.g., an additional variable capturing some old value,
modified later and checked afterwards, or a function parameter name that
is only used within a DBG_ASSERT).  This is a lot of work to clean up
(see the easy hack above).

- I replaced the single SAL_LOG_LEVEL (with values 0, 1, 2) with
individual defines SAL_LOG_INFO and SAL_LOG_WARN.  This makes it easier
to replace the code mentioned above, going from

   #ifdef DBG_UTIL
     int old = x;
   #endif
     ... // modify x
     DBG_ASSERT(x > old);

to

   #if defined SAL_LOG_WARN // instead of: SAL_LOG_LEVEL >= 1
     int old = x;
   #endif
     .. // modify x
     SAL_WARN_IF(x <= old, "...");

(Where for such a cheap initialization like "int old = x;" it would also
work to have that variable included unconditionally, followed by a
"(void) old; // avoid warnings"; but there are also cases of more
expensive initialization, that you would likely want to keep excluded
from production builds.)

- The SAL_INFO/WARN messages are now defined to be always UTF-8, and
rtl/oustringostreaminserter.hxx always converts to UTF-8.  That
minimizes information loss and would potentially enable use of
SAL_STREAM for construction of UNO exception messages, see the recent
thread about that.  However, I do not bother to convert the data output
to stderr from UTF-8 to any other encoding.

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg
Hi,

On Fri, Nov 18, 2011 at 03:25:16PM +0100, Stephan Bergmann wrote:
> For one, there are two sets of functionality that cater for the same
> needs.  One is osl/diagnose.h, the other is tools/debug.hxx.

There is also 3/4 of a log4j reimplementation in extensions/source/logging.
Just saying ...

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Stahl-2 Michael Stahl-2
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

In reply to this post by sberg
On 18/11/11 15:25, Stephan Bergmann wrote:
> The downside of that mixture is that the useful debugging technique of
> aborting upon detection of a violated invariant is not available.---If
> you make OSL_ASSERT (and OSL_ENSURE, OSL_FAIL, etc.) abort, it will
> abort far too often for mundane warnings for it to be useful.

indeed, the current state of tens of thousands of assertions firing on a
subsequentcheck run is pretty awful, this needs to be sorted out to have
the really bad assertions actually demand developer attention by calling
abort.

[...]

> 3  Furthermore, there is sometimes demand for additional, debug-only
> code.  In general, that would be safety-check additions that are
> considered too expensive to be included in production builds (e.g., code
> that iterates over a data structure to check invariants, or additional,
> redundant fields within data structures).  Enabling such additional code
> potentially affects compatibility.
>
> Such additional code is currently controlled via OSL_DEBUG_LEVEL et al.
>   OSL_DEBUG_LEVEL==1 is generally used for additions that do not affect
> compatibility (as it is enabled by both --enable-debug and
> --enable-dbgutil).  OSL_DEBUG_LEVEL==2, DBG_UTIL (defined upon
> --enable-dbguitl) and privately invented defines in certain parts of the
> code (to be set manually by knowledgeable developers) are used for
> additions that affect compatibility or that are considered too specific
> for general inclusion.  Either because they are too expensive even for
> every --enable-dbgutil build, or because they produce excessive log
> information (for which case the below new log functionality offers a
> better solution).

one requirement i would have on conditional compilation is that, whether
--disable-dbgutil or --enable-dbgutil, objects built with debug=t
(resulting in OSL_DEBUG_LEVEL being set to non-zero) should always be
binary compatible with objects built without debug=t.

this makes e.g. tracking down bugs introduced by mis-optimisation much
easier; i think we are in agreement on this point.

> This can probably be reduced to three cases:
>
> #if OSL_DEBUG_LEVEL != 0  for additional code that does not cause
> incompatibilities (if there is still demand for such code; the new log
> functionality will remove the need for such code in many cases).  This
> effectively reduces OSL_DEBUG_LEVEL to a binary switch (so the make
> dbglevel=x feature can be removed; and OSL_DEBUG_LEVEL could potentially
> be renamed---but that would probably not be worth it).
>
> #if defined DBG_UTIL  for additional code that causes incompatibilities.

i think i've seen members of SwDoc being added with:
 #if OSL_DEBUG_LEVEL > 1
 #if OSL_DEBUG_LEVEL > 0
this kind of thing always struck me as wrong: it should be DBG_UTIL,
will try to clean that up a bit...

> #if defined MY_SPECIAL_DEBUG  (replaced with actual defines, varying
> across the different code modules) for those special cases where always
> enabling the additional code is deemed to expensive in general.
> (However, for those special cases where the additional code produces
> excess log information, see below.)

in these cases it is expected that the macro only affects a single
module, and the author knows what he/she is doing, so no guarantees on
binary compatibility required.

>>     Whether these macros generate any log output is controlled in a two-stage
>>     process.
>>
>>     First, at compile time the macro SAL_LOG_LEVEL controls whether these macros
>>     expand to actual code, or to no-ops.  SAL_LOG_LEVEL must expand to an
>>     integral value 0, 1, or 2.
>>
>>     If SAL_LOG_LEVEL is 0, neither the INFO nor the WARN macros produce code.
>>     If SAL_LOG_LEVEL is 1, only the WARN macros produce code.  If SAL_LOG_LEVEL
>>     is 2, both the INFO and the WARN macros produce code.

hmmm... i wonder if it makes sense to not distinguish between warnings
and info at compile-time (given that it is only active on debug builds
anyway), so it is not required to recompile a module to get full debug
output...

>>     Second, at runtime the environment variable SAL_LOG further limits which
>>     macro calls actually generate log output.  The environment varialbe SAL_LOG
>>     must either be unset or must match the regular expression

>>     If the environment variable is unset, "+WARN" is used instead (which results
>>     in all warnings being output but no infos).  If the given value does not
>>     match the regular expression, "+INFO+WARN" is used instead (which in turn
>>     results in everything being output).

... with the runtime default being to output only warnings, as above.

>>     A given macro call's level (INFO or WARN) and area is matched against the
>>     given switches as follows:  Only those switches for which the level matches
>>     the given level and for which the area is a prefix (including both empty and
>>     full prefixes) of the given area are considered.  Log output is generated if
>>     and only if among the longest such switches (if any), there is at least one
>>     that has a sense of "+".  (That is, if both +WARN.foo and -WARN.foo are
>>     present, +WARN.foo wins.)
>>
>>     For example, if SAL_LOG is "+INFO-INFO.foo+INFO.foo.bar", then calls like
>>     SAL_INFO("foo.bar", ...), SAL_INFO("foo.bar.baz", ...), or
>>     SAL_INFO("other", ...) generate output, while calls like
>>     SAL_INFO("foo", ...) or SAL_INFO("foo.barzzz", ...) do not.

(de-)activating by module is very useful, as everybody who has been
overwhelmed by the full debug output from e.g. sal would probably agree...

in summary, sounds like a good plan :)

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Assertions and Logging

Hi Michael, Stephan, all,

On Tue, Nov 22, 2011 at 12:46:52PM +0100, Michael Stahl wrote:
> one requirement i would have on conditional compilation is that, whether
> --disable-dbgutil or --enable-dbgutil, objects built with debug=t
> (resulting in OSL_DEBUG_LEVEL being set to non-zero) should always be
> binary compatible with objects built without debug=t.
>
> this makes e.g. tracking down bugs introduced by mis-optimisation much
> easier; i think we are in agreement on this point.

Full agreement here.

> i think i've seen members of SwDoc being added with:
>  #if OSL_DEBUG_LEVEL > 1
>  #if OSL_DEBUG_LEVEL > 0
> this kind of thing always struck me as wrong: it should be DBG_UTIL,
> will try to clean that up a bit...

A bit of digging in gits history shows that to be a blunt removal commit of
DBG_UTIL in sw in 2010. I cant make any sense of it, as it completly broke non
DBG_UTIL debug builds for no gain. Can anyone enlighten me on this?

> hmmm... i wonder if it makes sense to not distinguish between warnings
> and info at compile-time (given that it is only active on debug builds
> anyway), so it is not required to recompile a module to get full debug
> output...

Agree. The only valid reason for not having all debug tools compiled in is
runtime performance and binary size. Once you are debugging those are mostly
irrelevant.

> in summary, sounds like a good plan :)

yep.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Next » 123