handle configuration changes with more code coverage

[nashif]

Reported by Inaky Perez-Gonzalez:

As a developer, I want to be able to switch on and off different parts of the code at build time, but at the same time, maximize the code coverage of compiles to avoid bit rot.

Currently a lot of #ifdef sections are used, which cause hardship in validation of the validity of code (eg: when an API is changed in somefile.h that is used in anotherfile.c, but that usage is only enabled when CONFIG_OPTION_X is enabled). Although the solution "test with CONFIG_OPTION_X enabled" is the obvious and correct answer, it quickly becomes apparent it does not scale when the number of config options and combinations skyrocket.

We have been working on some proposals to handle these cases, which enable the code to be compiled all the time and reduces the exposure of #ifdefs to way smaller corners of the code that will actually have the compile determine if the compiled code has to be optimized out (and thus there is no code size impact) or kept.

This task is to track the implementation of the generic frameworks for doing so across different drivers and subsystems.

Initial candidates:

• power management hooks
• reentrancy protection

(Imported from Jira ZEP-578)

[nashif]

by Inaky Perez-Gonzalez:

Initial Proof of concept code for a device driver whose code paths are enabled/disabled for different sub-subsystems with the code being always compiled and out of #ifdef blocks:

/*
 * This is a proof of concept for the switching on/off code based on
 * config
 *
 * - still builds as much code as possible for improved code coverage
 * - still optimizes out the code that otherwise would have been wiped
 *   away by #ifdef switches
 *
 * In this example I am working with device (and driver) XYZ, for
 * which we have two "optional" subsystems to handle:
 *
 *   -kernel-wide power management (whose implementation functions are
 *    part of the driver ops)
 *  - driver-specific reentrancy protection.
 *
 * Build the POC with:
 *
 *   cc -O2 -g -Wall -c config-pm.c
 *
 * Add -DPM_ENABLED=1 and/or DRP_ENABLED=1 to configure in/out the
 * "power management' and "reentrancy protection"
 *
 * See the sizes and objects with:
 *
 *   nm -S config-pm.o | sort -k4
 *
 * It kinda enforces to put the "optional" context variables inside an
 * struct that then it is embedded (or not, dep on config) into the
 * driver data structure for the device instance.
 *
 *   It could be done without (by just using the variable straight)
 *   but I like that doing it like that creates a pattern we can
 *   easily reproduce in other subsystems.
 */

#include <stddef.h>

/* Stub semaphores, for the example */

struct nano_sem {
    int sem;
};
void nano_sem_init(struct nano_sem *);
void nano_sem_give(struct nano_sem *);
void nano_sem_take(struct nano_sem *, int);
#define TICKS_UNLIMITED -1

/*
 * Let's say this would be the power management subsystem
 */

#ifdef PM_ENABLED

#warning PM is enabled
#define DECLARE_ON_PM(a) a
#define GET_PM_DATA(_type, dev, field)                \
    (&((_type *)dev->driver_data)->field)
#define INIT_PM(field, fn)        \
    .field = fn,

#else

#warning PM is disabled
#define DECLARE_ON_PM(a) struct { }
#define GET_PM_DATA(_type, dev, field) (NULL)
#define INIT_PM(field, fn) /* nothing */

#endif

/*
 * This would be include/device.h and friends
 *
 * These would be your general definitions in the device subsystem
 */

/**
 * allow passing an argument to a macro that contains one or more
 * commas.
 *
 * MACRO(__one_arg(a, b))
 */
#define __one_arg(...) __VA_ARGS__

#define _str(s) #s
#define str(s) _str(s)

struct device {
    const char *name;
    void *driver_data;
    /* operations */
    void (*init)(struct device *);
    void (*op1)(struct device *);
    /* for the sanity of the documentation processor,
     * it might be better to just #ifdef here */
    DECLARE_ON_PM(void (*pm_op1)(struct device *));
    DECLARE_ON_PM(void (*pm_op2)(struct device *));
    DECLARE_ON_PM(void (*pm_op3)(struct device *));
};

/*
 * syntactic sugar to avoid "unused static"
 *
 * In here we list the functions that an area/subsystem might or not
 * might use but that the compiler may optimize out.
 *
 * This variable is all static consts and nobody will use it, so all
 * functions that otherwise would be listed as unused and the compiler
 * would warn, shall be listed here. When it comes time to assemble
 * the object file, the compiler will optimize away this variable away
 * and any function that is listed here that is not used by anyone
 * else.
 */
#define UNUSED_FNS(area, ...)            \
static void * const __unused_##area[] = {    \
    __VA_ARGS__                \
}

/*
 * Define a device object
 *
 * Note we include all the functions specific to a subsystem that
 * might be being enabled in runtime or not. We still include them
 * because we want to build them for code coverage.
 *
 * Note in this exmple, _init and _op1 are the common driver
 * operations, _pm_* are power management specific.
 */
#define DEVICE(_name, _data,            \
        _init, _op1,            \
        _pm_op1, _pm_op2, _pm_op3)    \
                        \
UNUSED_FNS(pm, _pm_op1, _pm_op2, _pm_op3);    \
                        \
const struct device devops_##_name = {        \
    .name = str(_name),            \
    .driver_data = _data,            \
    .init = _init,                \
    .op1 = _op1,                \
    INIT_PM(pm_op1, _pm_op1)        \
    INIT_PM(pm_op2, _pm_op2)        \
    INIT_PM(pm_op3, _pm_op3)        \
}

/*
 * This would be the driver code
 *
 */

/*
 * Let's say this would be the reentrancy protection "subsystem",
 * specific to the driver (hence why it is here versus in a header
 * file). Note it could be made generic too, as the PM case.
 *
 * RP needs no fields in the struct device (PM does)
 *
 * Note in here we will use the constant rp_enabled to help optimize
 * out because there are no function pointers to worry about (thus no
 * INIT_RPs). If we made it generic, we'd have to use a trick like
 * PM's to get the code optimized out.
 */

#ifdef RP_ENABLED

#warning RP is enabled
static const int rp_enabled = 1;
#define DECLARE_ON_RP(a) a
#define GET_RP_DATA(_type, dev, field)        \
    (&((_type *)dev->driver_data)->field)
#define INIT_RP(field, val)            \
    .field = val,

#else

#warning RP is disabled
static const int rp_enabled = 0;
#define DECLARE_ON_RP(a) struct { }
#define GET_RP_DATA(_type, dev, field) (NULL)
#define INIT_RP(field, val) /* nothing */

#endif

/*
 * Specific information to PM for the device; the idea is to always
 * keep this structure the same, even if PM is disabled, so that
 * all the code can be compile tested -- it will be optimizied out if
 * disabled.
 */
struct xyz_pm_data {
    int field1;
    char field2;
};

/*
 * Specific information to reentrancy protection for the device
 */
struct xyz_rp_data {
    struct nano_sem sem;
};

/*
 * Driver specific data
 */
struct xyz_data {
    unsigned long someval;
    DECLARE_ON_PM(struct xyz_pm_data pm);
    DECLARE_ON_RP(struct xyz_rp_data rp);
};

/* RP operations */
static
void xyz_rp_critical_region_init(struct device *dev)
{
    struct xyz_rp_data *rp_data = GET_RP_DATA(struct xyz_data, dev, rp);
    if (!rp_enabled)
        return;
    nano_sem_init(&rp_data->sem);
    nano_sem_give(&rp_data->sem);
}

static
void xyz_rp_critical_region_start(struct device *dev)
{
    struct xyz_rp_data *rp_data = GET_RP_DATA(struct xyz_data, dev, rp);
    if (!rp_enabled)
        return;
    nano_sem_take(&rp_data->sem, TICKS_UNLIMITED);
}

static
void xyz_rp_critical_region_end(struct device *dev)
{
    struct xyz_rp_data *rp_data = GET_RP_DATA(struct xyz_data, dev, rp);
    if (!rp_enabled)
        return;
    nano_sem_give(&rp_data->sem);
}

/*
 * This function shall always compile, even if pm is disabled, to
 * do code coverage. However, the compiler shall optimize it out when
 * pm is disabled
 */
static
void xyz_pm_op1(struct device *dev)
{
    struct xyz_data *data = dev->driver_data;
    struct xyz_pm_data *pm_data = GET_PM_DATA(struct xyz_data, dev, pm);
    xyz_rp_critical_region_start(dev);
    data->someval = 0;
    pm_data->field1 = 3;
    xyz_rp_critical_region_end(dev);
}

static
void xyz_pm_op2(struct device *dev)
{
    struct xyz_pm_data *pm_data = GET_PM_DATA(struct xyz_data, dev, pm);
    pm_data->field2 = 3;
}

static
void xyz_pm_op3(struct device *dev)
{
    struct xyz_data *data = dev->driver_data;
    xyz_rp_critical_region_start(dev);
    data->someval = 3;
    xyz_rp_critical_region_end(dev);
}

/* other device/driver operations */

static void xyz_init(struct device *dev)
{
    struct xyz_data *data = dev->driver_data;
    xyz_rp_critical_region_init(dev);
    data->someval = -1;
}

static void xyz_op1(struct device *dev)
{
    struct xyz_data *data = dev->driver_data;
    xyz_rp_critical_region_start(dev);
    data->someval = 0;
    xyz_rp_critical_region_end(dev);
}

static struct xyz_data xyz_data_instance1 = {
    .someval = 4,
    INIT_PM(pm, __one_arg({
        .field1 = 4,
        .field2 = 'c'
    }))
};

DEVICE(instance1, &xyz_data_instance1,
       xyz_init,
       xyz_op1,
       xyz_pm_op1, xyz_pm_op2, xyz_pm_op3);

[nashif]

by Inaky Perez-Gonzalez:

Current identified limitations of the PoC that have to be explored further:

• what happens when a driver is implemented in multiple .c files and we can't rely on the compiler to optimize out stuff that is statically defined?

• when the "subarea" data is just a variable, it seems kind of overkill to define a struct for it -- I like that it creates an stable design pattern for all to reproduce, but it would be a good idea to also expose how it can be done when there is just a single variable that doesn't warrant a whole struct being introduced.

[nashif]

by Andrew Boie:

Are we trying to abolish the use of #ifdef?

[nashif]

by Iván Briano:

I don't think the idea is to abolish #ifdef, but to avoid relying on it so much to remove unused code. I've been bit a few times by things that break because one CONFIG option is not being exercised and I think that, at least for kernel wide stuff, it would be good to have a way to always check the code builds, at least.

[nashif]

by Inaky Perez-Gonzalez:

ACK to Ivan's; we are looking more at minimizing the amount of code under #ifdef.

The least code that uses pre-processor constructs, the more chances we have for the C compiler to catch inconsistencies.

[nashif]

by Andrew Boie:

I think I see where you are going with this. I imagine this could be part of a larger effort to overhaul how we do test cases, such as being discussed on the mailing list.

[nashif]

by Inaky Perez-Gonzalez:

I think this is parallel to test cases.

One of the objectives of laying out the code like this (and BKMs have to be created, so people can follow references) is that on existing test cases, for doing coverage tests, we don't necessarily have to enable everything that is possible to enable but still it would compile as much code as possible, which will be removed out by compiler optimization.

[nashif]

by Andre Guedes:

For the PM-related part, why not simply disable function-not-used warning and rely on ffunction-sections + gc-sections options to get rid of suspend and resume functions when PM support is not enabled?

[nashif]

by Inaky Perez-Gonzalez:

warnings are there for a reason--if we disable for one, it would catch other instances where it clearly is a problem and thus we'd have code creep without being able to act properly on it (even if the linker would catch it).

The mechanism used has to involve the developer knowing what's going on rather than her/him just blindly doing stuff. There is a way to do it that involves little #ifdef magic, see

https://gerrit.zephyrproject.org/r/#/c/3414/2/drivers/pwm/pwm_qmsi.c

we have planned the same methodology for the PM subsystem, a little bit enhanced to match the needs of it, but that will be for 1.6, as it also implies a lot of cleanup in a few things.

[nashif]

by Andrew Boie:

I think Andre Guedes 's suggestion has merit. The linker throws away functions that aren't called or referenced anywhere else. I have no objections to adding -Wno-unused-functions to the defaults CFLAGS.

Inaky Perez-Gonzalez keep in mind that --function-sections / gc-sections is not default compiler/linker behavior. So it's unsurprising that -Wunused-functions is enabled by default.

I see an advantage here that all the code gets compiled and we don't have to impose any esoteric coding style conventions.

[nashif]

by Andrew Boie:

I also think it's worth noting that there's already precedent for adjusting warning options, if you look at toplevel Makefile around line 650 or so you'll see lots of adjustments to other warnings being made.

[nashif]

by Inaky Perez-Gonzalez:

We still do some bits of #ifdef magic to avoid swelling of the data structures.

If function (eg) driverxyz_pm_save() requires some data context (struct devicexyz_pm_context to work,the context has to be placed somewhere (likely inside the struct devicexzy_context).

If the codepaths are not called, the compiler or linker will optimize out, but not the context; wherever we place it, it will still be there.

Hence your magic becomes

struct driverxyz_context {
    ...
#ifdef CONFIG_PM
   struct driverxyz_pm_context pm;
#endif
   ...
};

#ifdef CONFIG_PM
#define DRIVERXYZ_PM_CONTEXT(driverxyz_context) (&(driverxyz_context)->pm))
#else
#define DRIVERXYZ_PM_CONTEXT(driverxyz_context)  NULL
#endif

your #ifdef surface has been reduced to almost minimum, the code compiles all the time and the functions will never be called and thus optimized automatically by the compiler, with no other magic having to happen in the linker.

note in this very particular case some more magic is needed for the function pointers because PM ops are accessed via them; the linker has NO way of telling if they are being called or not (it can be modified in runtime, dumb as that might be) and then it'd be hyperspacing into an unknown section of code.

Look at the poc code in the first comment; it explains everything in detail and covers most (if not all) situations we are to face within Zephyr. For granted, the style and convention can be altered, I've used the default LK style.

[nashif]

by Ramesh Thomas:

In both the methods (-Wno-unused-functions and the const method of Inaky), is the compiler actually compiling the code before optimizing? Especially if it sees a reason to warn of unused functions, then why would it compile them?

The decision to not link in by the linker, comes in at a later stage, provided the function has been compiled and is included in the object file. So we need to make sure the code actually gets compiled if code coverage is the requirement.

[nashif]

by Andrew Boie:

is the compiler actually compiling the code before optimizing? Especially if it sees a reason to warn of unused functions, then why would it compile them?

it still builds them even if it thinks they are unused, hence the warning you can test this yourself by creating an unused function and put a syntax error in it.

[nashif]

by Ramesh Thomas:

You are right. I assumed it wouldn't go through the trouble of compiling. I think the theory is that it is not the compiler's responsibility to leave out unused functions.

[nashif]

by Andy Ross:

My attention was just pointed to this bug. I'll take it; the patch here: https://gerrit.zephyrproject.org/r/#/c/4871/ is more or less responsive to exactly that.

Basically there's a trick in Linux for turning a CONFIG_* variable that may or may not be defined into a literal "1" or "0" that can be seen by the compiler and optimized away in a clean manner without preprocessor muckery.