[cairo] How soon to call cairo_error?

[Chris Wilson]

Carl Worth (cworth at cworth.org) said: 
> I have three concerns when looking at this commit:
> 
> 1) Cairo itself might handle the non-success status, so the
>    lowest-level assignment might not actually be an error at all, (so
>    reporting it with _cairo_error is just plain wrong).
> 
> 2) We previously had tried to call _cairo_error as high as possible
>    within cairo's stack, (as close to the user code as possible), so
>    this change may introduce multiple calls to _cairo_error for a
>    single error. That could be distracting.
> 
> 3) The deeper calls to _cairo_error mean that the user will be plunged
>    deeper into cairo's guts when debugging[*]. That could be annoying.
> 
[snip]
> 
> If we are extremely confident that there aren't more, then that's
> fine. (By the way, Chris, how exhaustive has your error-checking work
> been? Do you have coverage reports to show us? Or in general, have you
> documented the strategy that has led to the phenomenal number of
> patches you've made for cairo?)

It's been on my TODO list for too long... I've just got a couple more
fixes for the pdf backend and to present the extended cairo-test for
review. However, since that mechanism is only a memory fault injector we
rely on test-cases to generate the other error conditions. In practise
the error paths only add a few percent to lcov - the worst offenders
there is lack of test coverage for type subsetting. (And I'm limited to
testing on platforms supported by valgrind, so I've never looked at
whole sections of code.) Again, I've been meaning to push the results
from lcov to annarchy.fd.o for some time.

> The other two points are cosmetic to some extent. It really would make
> it easier for users to debug their own code if given as little of
> cairo's internals as possible. On the other hand, if we're missing any
> calls to _cairo_error on any error-path then that's a fatal blow to
> this debugging technique. I'm not confident that we weren't missing
> any before this patch, so I'm more than willing to accept the
> less-kind behavior as a cost of making sure we don't miss any.

I've found the fine-grained _cairo_error() very useful, as I'm sure
have, or perhaps will have, a number of other Cairo developers - I
recall Behdad complaining a couple of times about how hard it was to
find precisely where the error occurred. And when it's coupled with a
stacktrace it becomes much more useful. I've usually just plugged a
VALGRIND_PRINTF_BACKTRACE() into _cairo_error(), but there were murmurs
about including util/backtrace-symbols for supported architectures.

The fine-grained approach should not hinder users of the library either,
as the aim should be for early detection of errors so that if cairo is
misused then the user is not present with _cairo_error from the bowels
of the library. This still exposes the user to _cairo_error from the
boundary between cairo and lower libraries, but those should fail only
in exceptional cases where cairo itself has misused the library.

Perhaps a way around the cosmetic issue, is to suggest to users that
they set the breakpoint on _cairo_set_error() which will allow them to
pin-point the function they called without seeing any of cairo's
internals (but the difference would only be a few frames in the bt).
 
> But if there is any way to be confident that we're hitting all error
> paths, (some sparse magic or something?), I would prefer to have the
> _cairo_error calls as high up as possible.

I'd guess a semantic analysis tool might have a chance at spotting
missed error chains - has there been any progress with someone in a
position to forward reports from coverity? At times I've considered
adding some function epilogues to check that if a status return is not
SUCCESS then _cairo_error() should have been called... But then it's
back to case (1), where an error status might be handled correctly and
should not be propagated back to the user.
 
> So no real objection to the change here. Just some food for thought.
That's a relief - I still feel that the fine-grained sprinkling of
_cairo_error, coupled with the eventual CAIRO_DEBUG=backtrace (or some
such), will be useful to many people and pay for the effort in
maintaining in the coverage of _cairo_error() - i.e. it will save people
more time in debugging their problems than it will cost us in
marking up the errors.
--
Chris Wilson