Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Mar 14, 2016

Conversation

@sergeuz
Copy link
Member

commented Mar 9, 2016

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 comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

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.

#elif __STDC_VERSION__ >= 199901 && !__STRICT_ANSI__ // C99 with GNU extensions

#define DYNALIB_FN(tablename,name,type) \
__builtin_choose_expr(__builtin_types_compatible_p(type, __typeof__(name)), name, sizeof(struct { \

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Mar 9, 2016

Contributor

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.)

This comment has been minimized.

Copy link
@sergeuz

sergeuz Mar 9, 2016

Author Member

Done.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

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 sergeuz force-pushed the sergeuz:abicheck branch from 8c8e0ab to 36a0dc3 Mar 10, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

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

@m-mcgowan m-mcgowan added this to the 0.5.x milestone Mar 14, 2016

m-mcgowan added a commit that referenced this pull request Mar 14, 2016
Merge pull request #895 from sergeuz/abicheck
dynalib: compile-time check for certain types of ABI breaking changes

@m-mcgowan m-mcgowan merged commit 93f96e1 into particle-iot:develop Mar 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

Fixes issue #875

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.