[Qemu-devel] [RFC] Introducing CoroCheck and proposal for a blocking_fn annotation

* [Qemu-devel] [RFC] Introducing CoroCheck and proposal for a blocking_fn annotation
@ 2013-09-06 14:30 Gabriel Kerneis
  2013-09-06 14:30 ` [Qemu-devel] [RFC] Introduce " Gabriel Kerneis
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Gabriel Kerneis @ 2013-09-06 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, stefanha, pbonzini, Gabriel Kerneis, charlie

During his GSoC on writing a CPC [1] backend for QEMU, Charlie Shepherd
started to explicitly annotate coroutine_fn [2]. On aspect of this
refactoring work has been to split "dynamic" functions (that used
qemu_in_coroutine) into a blocking version and a coroutine one. It has
been agreed that it was a positive change, at least by some people who
considered dynamic function a bit hackish, but it has an obvious
downside: if one mistakenly call the blocking version from a coroutine
function, the main loop might block for a long time. The risk is
mitigated by suffix blocking function's name with _sync and other
tricks, but it is not a very reliable, long-term solution.

To help Charlie's large-scale refactoring, I have written a small tool,
CoroCheck [3], that performs a static analysis of QEMU and checks that
coroutine_fn annotations actually make sense.

More precisely, here is what CoroCheck does for each file of QEMU:
- assuming annotations in headers and typedef are correct, compute the
  minimal set of functions that should be annotated with coroutine_fn,
- warn about function pointer casts and assignments that introduce or
  remove coroutine_fn annotations,
- warn about missing or spurious coroutine_fn annotations,
- produce a .dot file that can be processed with graphviz to produce a
  pdf of the annotated call graph (with wrong annotations showing up in
  red - this still needs to be documented properly).

To learn how to analyse QEMU with CoroCheck, see CoroCheck's README [4].

    On a related note, #define coroutine_fn and #define blocking_fn
    should probably be surrounded by #ifndef to allow easy redefinition
    using ./configure --extra-cflags="-Dcoroutine_fn='…'"  I'll leave
    that follow-up patch as an exercise for Charlie ;-)

Charlie asked me to add a functionality to CoroCheck enabling him to
annotate blocking functions, and warn if they were called from coroutine
ones. CoroCheck now also supports this functionality.

For the reasons outlined in my first paragraph, I think that annotating
blocking functions is important for QEMU at large. It would be a pity
that Charlie wastes his time annotating functions locally, and that this
valuable piece of documentation is lost when he submits his patches.

The attached patch does not actually annotate any function, but it paves
the way for such future annotations. This is more a design choice than a
technical issue, and I hope a consensus can be reached soon enough to
allow Charlie to use it in his next patch series.

Note that CoroCheck has been written as a plugin to CIL [5]. Contrary to
CPC, which is still somewhat of a prototype (although a pretty good
one!), CIL is a solid piece of software, packaged in both Fedora and
(very soon) Debian. CoroCheck makes use of the CIL plugin facility which
has not made its way into a released version yet, but this should happen
in the next few months. Therefore, in a not-too-distant future, it is
reasonable to imagine that static checking of QEMU coroutine_fn
annotations could be (an optional) part of QEMU test suite. Adding
blocking_fn annotations would make even more sense in this context.

Best regards,
Gabriel

[1] https://github.com/kerneis/cpc
[2] http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg00529.html
[3] https://github.com/kerneis/corocheck
[4] https://github.com/kerneis/corocheck#qemu
[5] http://kerneis.github.io/cil/

Gabriel Kerneis (1):
  Introduce blocking_fn annotation

 include/block/coroutine.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 6+ messages in thread