dynalib: compile-time check for certain types of ABI breaking changes

[sergeuz]

This patch updates DYNALIB_FN() macro to enable compile-time check of the signatures of the dynamically exported functions. Solution is not ideal, but it allows to catch some of the typical errors early, and doesn't require much of extra typing work.

This patch also adds signatures for all currently exposed functions. enumerate_build_matrix.sh script completes normally.

[m-mcgowan]

This is excellent! A really nice implementation, especially that it works for C as well. 👍

One other source of problems is changes to the index of a given function, e.g. by inserting a new function in the middle of the list. How about we add an index in the first parameter to DYNALIB_FN() so that inserting a function in the middle of the list would require changing all subsequent indexes? For most dynalibs this will be simple since - there there are a couple (hal and comms) that have conditional compilation which will affect the generated indices.

[m-mcgowan]

could you replace the 2nd argument name with (const void*)&name, just so that we are certain there is no code change between this and the previous version? (That's how name was used in DYNALIB_FN previously.)

[sergeuz]

Done.

[sergeuz]

I was thinking about indices as well, but I'm not sure how this can be implemented, as C++ doesn't support designated array initializers, contrary to GNU C. Should the index simply be a dummy argument?

[m-mcgowan]

I was thinking like DYNALIB_FN(idx, table, name), and the macro would check the idx value against the current COUNTER value. (We already compute the index as part of the function import.)

[sergeuz]

The problem is that I have to evaluate asserting condition twice in case of plain C, because obvious _Static_assert(0, ...) gets fired immediately despite the docs saying that alternative branch of __builtin_choose_expr() is never evaluated. I'll check if I can use something like __COUNTER__ - 1 in this case.

[sergeuz]

ok, here is how additional index check can be implemented in C:

#define _DYNALIB_FN(index, counter, tablename, name, type) \
    __builtin_choose_expr(index == counter, \
        __builtin_choose_expr(__builtin_types_compatible_p(type, __typeof__(name)), (const void*)&name, \
            sizeof(struct { _Static_assert(__builtin_types_compatible_p(type, __typeof__(name)), \
                "Signature of the dynamically exported function has changed");})), \
        sizeof(struct { _Static_assert(index == counter, \
            "Index of the dynamically exported function has changed");})),

#define DYNALIB_FN(index, tablename, name, type) \
    _DYNALIB_FN(index, __COUNTER__, tablename, name, type)

My concern here is do we really want this implemented? :) Conditional compilation seems to significantly affect readability and maintainability of the code:

DYNALIB_BEGIN(test)
#ifdef HAS_FOO
DYNALIB_FN(0, test, foo, void())
#define OFFS 1
#else
#define OFFS 0
#endif
DYNALIB_FN(OFFS + 0, test, bar, void())
DYNALIB_END(test)

[m-mcgowan]

I can appreciate the conditional compilation affects readability, but this code isn't read all that often but mostly written - any new functions will simply be +1 on top of the index of the previous function in most cases. Putting a function in the wrong place is easily done (several people have already tried!) so an automated test to catch this seems to be more valuable than the hit to readability.

I'm also happy to have a compromise where the index of conditionally included functions (and those appearing after) are not checked. At least as a first iteration.

[sergeuz]

Well, the thing is already implemented and partially tested for both C and C++, I only need to update indices for all remaining functions. Not a big deal at all.

[sergeuz]

Added check for function indices and rebased this PR on top of current develop.

[m-mcgowan]

Fixes issue https://github.com/spark/firmware/issues/875