Thread safety

[a1ex]

While refactoring the menu code, I've noticed it became increasingly complex, so evaluating whether it's thread-safe was no longer an easy task (especially after not touching some parts of the code for a long time). The same is true for all other ML code. Not being an expert in multi-threaded software, I started to look for tools that would at least point out some obvious mistakes.

I came across this page, which seems promising, but looks C++ only. This paper appears to be from the same authors (presentation here), and C is mentioned too, so adapting the example is probably doable.

Still, annotation could use some help from a computer. So I found pycparser and wrote a script that recognizes common idioms from ML code (such as TASK_CREATE, PROP_HANDLER, menu definitions) and annotates each function with a comment telling what tasks call this function.

Therefore, if a function is called from more than one task, it must be thread-safe. The script only highlights those functions that are called from more than one task (that is, those that may require attention).

Still, I have a gut feeling that I'm reinventing the wheel. If you know a better way to do this, please chime in.

Source: https://bitbucket.org/hudson/magic-lantern/branch/thread-safety

Note: in DryOS, tasks == threads.

[g3gg0]

did not do any analysis like that yet, but i worked with parsing C code before.
the best and most stable solution was, using the C compiler to produce preprocessed output (option '-E') and use the resulting C code.

this will get rid of any preprocessor issues, especially function like macros that result in function calls or -definitions.

it also takes into account that parts of the code may or may not be used depending on specific defines.


so my solution was:
use the default make toolchain, "replace" the C compiler command so it will call a script, which:
 
a) produces the preprocessed output by calling smth like "$CC $CFLAGS -E"
b) executes an instrumenter that works on the preprocessed output (file passed to "-o")
b) compiles the resulting file as normal so the make toolchain works as intended (in my case i had to re-use the preprocessed output from -E that i've patched)

this way i was sure, i will not miss some corner cases where some ugly define made me miss parts of the code.

in this case, you could replace "gcc-arm-eaby-blah" with "analyse-wrapper" which then will be able to work as in-situ replacement of the compiler in a normal build process.
(t.b.h. not using the original preprocessor and -defines will make things ugly)

[a1ex]

Very good advice - almost got it working.

There are still a few places in the code where pycparser gets stuck (asm, gcc extensions), so the analysis requires a custom build step (which gives slightly different output). Most of the pycparser hiccups were solved with further postprocessing (sed followed by a second CPP pass), but a few ones are a bit harder to fix that way (such as the optimized MIN/MAX macros from imath.h). To find all these workarounds, grep for PYCPARSER under src/ and modules/.

Also got a minimal example of clang's thread safety checking in plain C (source).

[a1ex]

Some updates:


1) Got clang's thread safety analysis working in plain C. To get these warnings, compile with:

Code: [Select]

make clean; make PREPRO=y

How it works:

With PREPRO=y:
- it uses GCC preprocessor to create .i files (preprocessed C)
- it calls clang to check the .i files for thread safety (this is a fake target, you can trigger it manually with e.g. "make menu.t")
- it then calls gcc to build the object file from the preprocessed file
- this build process is a bit slower and a bit more verbose, but should generate the same binary output as a regular build (tested manually - the only difference is build date)

Without PREPRO=y (regular build):
- regular make will build as usual (.c -> .o)
- make *.t will build .c -> .i -> .t (so in this case, the preprocessed C is not used during compilation)

With PREPRO=y PYCPARSER=y:
- this uses some additional workarounds to enable analysis with pycparser (which gets stuck on gcc extensions used by us), so the binary resulted in this way may not be suitable for executing (though the differences are probably minor).

You can already see a few annotated sources. For example, lvinfo (which is pretty recent code) was already handling threads very well, so it got just a (false) warning. Other files are not so clean: mlv_lite gives some warnings; most of them are related from some vsync variables initialized in raw_rec_task (which should be fine, since vsync hook is not running during that initialization), or with updating resolution parameters (which is done from 3 different tasks, but there is some additional logic to make sure only one of them runs at a given time).

Still, only a tiny part of the code base was annotated and reviewed. It's a huge task, and the reward is not obvious at first sight - getting rid of some insidious bugs that happen with low probability and can be impossible to narrow down. But it's badly needed, as the project got pretty complex and is used by many for serious work.


2) Also added a test (in the selftest module) that checks various functions for thread safety (including a known unsafe function to make sure the test actually works).

This test works by creating 2 tasks that call the tested function in a loop, with some tricks to force a lot of context switches.

While writing this test, I've noticed something interesting, that should help getting rid of those race conditions in certain cases:

If we have two DryOS tasks with equal priorities, they will never interrupt each other, unless one task does some sort of yielding (msleep, waiting at semaphore / message queue / for event flags etc). That means, two DryOS tasks with equal priorities will use cooperative multitasking.

This property will probably no longer hold true on DIGIC 7 (dual core Cortex A9). Not tested on DIGIC 6.

The test I've ran to confirm this hypothesis was to run a long loop with NOPs (a few seconds) in 2 tasks and count the context switches. You can enable this test by defining TEST_EQUAL_PRIO in selftest.c and optionally logging context switches from our task_dispatch_hook (e.g. with CONFIG_TSKMON_TRACE, printf in qemu or anything else you consider useful).