Refactoring C to Remove Feature Flags

You’ve read the books on Refactoring, on working with legacy code, on Unit Testing and on TDD. Then you look at the codebase you’ve inherited, it’s written in C, and it’s riddled with conditional compilation. Where do you start?

screen-shot-2016-10-06-at-17-30-54

 

In years gone by feature flags were widely used in embedded systems as a means of having a common codebase shared across multiple devices. The devices varied in what hardware was present, what capacity there was in terms of RAM, ROM and performance. The devices also varied according to market demands, e.g. some features were only required on ‘premium’ products.

Now imagine how the codebase could have deteriorated over the years. Some of the code is forty years old, the code base has been targeted at fifty different hardware platforms, and at a marketing level there have been over one hundred different features. There are terrifying potential number of combinations of ways the software could be built.

How bad is your code? This command will show you how many different conditional statements there are in your code. Admittedly some will only be different because of whitespace, or because of the order of the flags.

grep --include=*.c --include=*.h -r -h '#if' . |sort -u | wc

I’ve been faced with a codebase containing 16000 different conditional include lines; codebases exist with many more than that. Where do you start? Should you start?

With this amount of conditional compilation, introducing Unit Testing may appear impossible, each test fixture can only be compiled with one combination of feature flags. You may be able to use it for new modules, but how about for maintenance? This article offers a step by step approach that I have used to remove feature flags, and remove conditional compilation from a large codebase (a few million lines).

As with all refactoring, there is a level of risk, the aim of these changes is to minimise the risk by taking baby steps and using a safety net.

Step 1 – Preparation - Repeatable builds

To remove a feature flag we need a test to know that we haven’t impacted the code. The method I like to use is to determine that the build produced a binary identical output before and after the change. Perform two complete builds and compare the build output. We need to get to the state where they are identical. There are multiple reasons why the output may vary, these need to be addressed before we attempt any refactoring:

  • Problem -  Time/Date of the build is included in the binary
  • Solution – Make the build use a fixed time for your test purposes. How you do this depends on how the time and date is injected into the build. Consider link time substitution of a fixed file, disabling that part of the makefile, or conditional compilation.
  • Problem – The version of a file or a checkout from the version control system is embedded in the build.
  • Solution – Be careful to checkout both copies from the same revision. If the revision information is in a single source file consider link time substitution to replace it with static values. If the information is in a single header file consider using the include path to prioritise a file with static values.
  • Problem – the file format of your binary includes the time that the build was performed.
  • Solution – Use another form of output to compare to decide the builds are identical, e.g. transform the output into a plain format such as .bin or SREC, or use a map file for comparison. (e.g. if using gnu, look at objcopy and strip)
  • Problem – the file format includes the paths of source files.
  • Solution – Use tools to strip debug information from the binary (e.g. if using gnu, look at objcopy and strip).  Or perform both builds in the same directory.

This process needs to be repeated for every build that is to be supported from your codebase. There may have been hundreds of products delivered, it is likely that only a small subset still require support. To be confident in your changes you must be sure that you are not impacting any of the current builds with your changes.

Step 2 – Identify redundant feature flags

We can identify a feature flag as redundant in each of these circumstances

  • It is defined to the same value on all supported platforms
  • It is undefined on all platforms
  • There are no longer any uses of the flag in the code
  • All uses of the feature flag are in sections of code removed by other feature flags

For cases 1, and 2, use the pre-processor to prove that your assumptions are correct by forcing a build that will fail only if your assumption is correct. (Choose the failing option because it is faster to test). For example, if you believe that FEATURE_A always has the value 1 on all platforms then add the following to a source file included early on in all builds

screen-shot-2016-10-06-at-17-37-17

 

Then verify that all of your builds fail. If they do then you know that this flag is safe to remove.

screen-shot-2016-10-06-at-17-37-43

Step 3 – Remove Feature Flags

Following on from the example above, assume that we have discovered that FEATURE_A always has the value 1 in all of the builds we need to support. How can we remove FEATURE_A when it may be mentioned in many of the thousands of files in our build? Removing by hand is going to be time consuming and worse error prone. 

To automate the process use unifdef. The command below invokes unifdef on every .c and every .h file below the current directory, and removes the conditional compilation related to FEATURE_A.

find . -name '*.[ch]' | xargs unifdef -DFEATURE_A=1 -m

Lets see what this did to our example function below. Not only has the #if FEATURE_A statement been removed, so to has #if FEATURE_A || FEATURE_B, unifdef was smart enough to determine that if FEATURE_A was defined the compound condition was always true.

screen-shot-2016-10-06-at-18-20-43

At this stage rebuild all of the applications, verify that none of the binaries have changed and commit the change to version control. Then repeat for the next feature flag. Lets see one more example, suppose FEATURE_B is always undefined, unifdef can be used to remove the feature with this command

find . -name '*.[ch]' | xargs unifdef -UFEATURE_B -m

Here we can see that the code guarded by #ifdef FEATURE_B has been removed as well as the feature flag.

screen-shot-2016-10-06-at-18-29-02

Verify that the binary output is identical, for all builds. Commit changes to version control and repeat.

Should you be worried about making these changes? What about the code that is being deleted, isn’t it valuable? No, it has no value. It isn’t included in any current builds, so it carries no current value. It adds confusion, and slows development, so it has cost and not value. If you ever have to look at what had previously been included in a feature you have removed, then your VCS provides a means for accessing that code. And if you have followed this process then you have a single commit for removal of each feature. 

I would repeat the above process for every feature flag that I suspect is identically defined in all live builds.

With the safety net of knowing all builds are binary identical, there is no risk of introducing bugs.

Step 4 – Removal of a feature flag that is in different states in different builds

Now if we consider the final conditional in our function, FEATURE_C; FEATURE_C is defined as 1 in some of our builds, and as 0 in others. How can we safely remove the conditional? Should we attempt to remove this conditional?

Personally I would attempt to remove this conditional only when I start working on code that is impacted by the conditional compilation, and not before.

It is unlikely that we are going to be able to make the changes to remove this feature and leave all builds binary identical, so we need another safety net to tell us that what we are doing has not had any nasty side effects.

To change the code away from using the pre-processor we must choose one of three other ways of varying the behaviour between builds.

  1. Compile Time Substitution
  2. Link Time Substitution
  3. Runtime Substitution

Lets assume we need to do some maintenance work in VeryLongFunction(). Before we try to make a functional change we want to get rid of this conditional compilation. And before we get rid of the conditional compilation we want tests to tell us that it is safe to do so.

So our first step is to create a test harness for this source file. Rather than re-state the process, look at James Grenning's article TDD How-to: Get your Legacy C into a Test Harness. In this test harness have FEATURE_C defined as 1, so that our conditionally included code is included in the test harness.

Now write some tests that prove the functionality of VeryLongFunction(), including a test that checks calls to wibble only occur if the previous functions have succeeded.

Great we have a test harness, now we can start refactoring. In this scenario, Extract Method looks like a good refactoring to try. Lets pull out all of the code inside FEATURE_C into a well named method (FeatureCWibbleIfOK isn't a great name, but it will do for our example, but do pay attention to the name you choose). We end up with something like:

screen-shot-2016-10-06-at-19-26-09

All of our tests still pass, we are good to continue. The next step in our refactoring is to open up a seam to allow us to substitute different behaviour. We move the function out into a new source file and create a new header, say feature_c.c, and feature_c.h. These files should be included into our test harness, and our tests all still pass.

Next step is to produce a test fixture to prove feature_c, once this is done we can simplify the the tests in our original test harness to prove that FeatureCWibbleIfOK is being called correctly, and remove feature_c from that test harness.

We are now at a point where we can substitute different behaviour and we need to decide with of our three possibilities we will use. In the first two cases we should develop a new test fixture, initially a copy of feature_c test harness, using a copy of feature_c.c, modify the test to expect the behaviour with FEATURE_C undefined, run the tests and observe them fail. Undefined FEATURE_C in the test harness and observe the test pass. You can then remove the FEATURE_C feature flag and code.

Compile Time Substitution

In compile time substitution we can use the include path to insert one of two different copies of feature_c.h, for example, one could have a plain prototype

int FeatureCWibbleIfOK(int ret);

and the other could have a null inline implementation.

inline int FeatureCWibbleIfOK(int ret){return ret;};

Link Time Substitution

For link time substitution, a second copy of feature_c.c may look a bit like

#include "feature_c.h"
int FeatureCWibbleIfOK(int ret)
{
    return ret;
}

Runtime Substitution

Here we presume that there is going to be some runtime check that allows us to determine if FEATURE_C is enabled. Use normal TDD methods to test drive this into your application.

Summary

Refactoring a large legacy code base that is riddled with conditional compilation is hard. However it can be safely achieved with care, allowing the code to be brought under control of test harnesses. You may never achieve full coverage of a test harness, but with care you should be able to bring the areas that you work on under control, get tests in place and gradually improve the quality and maintainability of the code.

It may be hard, but what other choices do you have

smiley-crossing-fingers

 

Posted in Software Development and tagged .

Leave a Reply

Your email address will not be published. Required fields are marked *