[PATCH] lib/igt_core: document the caveats of magic code blocks

[PATCH] lib/igt_core: document the caveats of magic code blocks

[Daniel Vetter]

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 lib/igt_core.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index ff4711785654..3e2350e9982a 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -83,6 +83,38 @@
  * To allow this i-g-t provides #igt_fixture code blocks for setup code outside
  * of subtests and automatically skips the subtest code blocks themselves. For
  * special cases igt_only_list_subtests() is also provided.
+ *
+ * # Magic Control Blocks
+ *
+ * i-g-t makes heavy use of C macros which serve as magic control blocks. They
+ * work fairly well and transparently but since C doesn't have full-blown
+ * closures there are caveats:
+ *
+ * - Asynchronous blocks which are used to spawn children internally use fork().
+ *   Which means that impossible control flow like jumping out of the control
+ *   block is possible, but it will horribly badly the i-g-t library code. And
+ *   of course all caveats of a real fork() call apply, namely that file
+ *   descriptors are copied, but still point at the original file. This will
+ *   terminally upset the libdrm buffer manager if both parent and child keep on
+ *   using the same open instance of the drm device. Usually everything related
+ *   to interacting with the kernel driver must be reinitialized to avoid such
+ *   issues.
+ *
+ * - Code blocks with magic control flow are implemented with setjmp() and
+ *   longjmp(). This applies to #igt_fixture and #igt_subtest blocks and all the
+ *   three variants to finish test: igt_success(), igt_skip() and igt_fail().
+ *   Mostly this is of no concern, except when such a control block changes
+ *   stack variables defined in the same function as the control block resides.
+ *   Any store/load behaviour after a longjmp() is ill-defined for these
+ *   variables. Avoid such code.
+ *
+ *   Quoting the man page for longjmp():
+ *
+ *   "The values of automatic variables are unspecified after a call to
+ *   longjmp() if they meet all the following criteria:"
+ *    - "they are local to the function that made the corresponding setjmp() call;
+ *    - "their values are changed between the calls to setjmp() and longjmp(); and
+ *    - "they are not declared as volatile."
  */
 
 static unsigned int exit_handler_count;
-- 
1.8.5.2

Re: [PATCH] lib/igt_core: document the caveats of magic code blocks

[Tvrtko Ursulin]

On 03/14/2014 07:18 AM, Daniel Vetter wrote:
[snip]
> + * # Magic Control Blocks
> + *
> + * i-g-t makes heavy use of C macros which serve as magic control blocks. They
> + * work fairly well and transparently but since C doesn't have full-blown
> + * closures there are caveats:
> + *
> + * - Asynchronous blocks which are used to spawn children internally use fork().
> + *   Which means that impossible control flow like jumping out of the control
> + *   block is possible, but it will horribly badly the i-g-t library code. And

I suggest to add a verb and reorder the "horribly badly" part of the 
sentence a bit. Or perhaps you tried to illustrate the program flow 
after jumping out of the control block? :)

> + * - Code blocks with magic control flow are implemented with setjmp() and
> + *   longjmp(). This applies to #igt_fixture and #igt_subtest blocks and all the
> + *   three variants to finish test: igt_success(), igt_skip() and igt_fail().
> + *   Mostly this is of no concern, except when such a control block changes
> + *   stack variables defined in the same function as the control block resides.
> + *   Any store/load behaviour after a longjmp() is ill-defined for these
> + *   variables. Avoid such code.
> + *
> + *   Quoting the man page for longjmp():
> + *
> + *   "The values of automatic variables are unspecified after a call to
> + *   longjmp() if they meet all the following criteria:"
> + *    - "they are local to the function that made the corresponding setjmp() call;
> + *    - "their values are changed between the calls to setjmp() and longjmp(); and
> + *    - "they are not declared as volatile."
>    */

So all variables which are getting initialised in igt_fixture should be 
moved out to file scope? Gcc does warn about potential clobbering 
although I haven't exactly gotten my head round understanding if and 
when it can really happen with igt_fixture.

Tvrtko

Re: [PATCH] lib/igt_core: document the caveats of magic code blocks

[Daniel Vetter]

On Fri, Mar 14, 2014 at 02:49:16PM +0000, Tvrtko Ursulin wrote:
> 
> On 03/14/2014 07:18 AM, Daniel Vetter wrote:
> [snip]
> >+ * # Magic Control Blocks
> >+ *
> >+ * i-g-t makes heavy use of C macros which serve as magic control blocks. They
> >+ * work fairly well and transparently but since C doesn't have full-blown
> >+ * closures there are caveats:
> >+ *
> >+ * - Asynchronous blocks which are used to spawn children internally use fork().
> >+ *   Which means that impossible control flow like jumping out of the control
> >+ *   block is possible, but it will horribly badly the i-g-t library code. And
> 
> I suggest to add a verb and reorder the "horribly badly" part of the
> sentence a bit. Or perhaps you tried to illustrate the program flow
> after jumping out of the control block? :)

I did frob the paragraph a bit before pushing. Can you please take a look
at what's committed and if you see some room for improvement send out
another patch?

> 
> >+ * - Code blocks with magic control flow are implemented with setjmp() and
> >+ *   longjmp(). This applies to #igt_fixture and #igt_subtest blocks and all the
> >+ *   three variants to finish test: igt_success(), igt_skip() and igt_fail().
> >+ *   Mostly this is of no concern, except when such a control block changes
> >+ *   stack variables defined in the same function as the control block resides.
> >+ *   Any store/load behaviour after a longjmp() is ill-defined for these
> >+ *   variables. Avoid such code.
> >+ *
> >+ *   Quoting the man page for longjmp():
> >+ *
> >+ *   "The values of automatic variables are unspecified after a call to
> >+ *   longjmp() if they meet all the following criteria:"
> >+ *    - "they are local to the function that made the corresponding setjmp() call;
> >+ *    - "their values are changed between the calls to setjmp() and longjmp(); and
> >+ *    - "they are not declared as volatile."
> >   */
> 
> So all variables which are getting initialised in igt_fixture should
> be moved out to file scope? Gcc does warn about potential clobbering
> although I haven't exactly gotten my head round understanding if and
> when it can really happen with igt_fixture.

Actually igt_fixture and igt_subtest, but usually it's igt_fixtures where
something gets set up and then clobbered. What I usually do is push them
out to global scope, but not at the top of the file but only right above
igt_main. That tends to avoid gcc warnings about shadowing.

Woud it be useful to add this bkm to the docs, too?

Thanks for the feedback, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch