Global variables are well known to be evil -- and you can read all about why that is in my free sample book chapter by that name. This posting gives a running example of changes that fix a common type of global variable.
Let's start with a pretty typical situation in a C program. You have a "globals.c" file that defines a mess of globals, including:
int g_ErrCount;
which might be used to tally the number of run-time errors seen by the system. I've used a "g_" naming convention to emphasize that is a global, which means that every .c file in the program can read and write this variable with wild abandon.
Let's say you also have the following places this variable is referenced, including globals.c just mentioned:
globals.c: int g_ErrCount; // define the variable
globals.h: extern int g_ErrCount; // other files include this
init.c: g_ErrCount = 0; // init when program starts
moduleX.c: g_ErrCount++; // tally another error
moduleY.c: XVar = g_ErrCount; // get current number of errors
moduleZ.c: g_ErrCount = 0; // clear number of reported errors
There are all sorts of risks with this approach... but let's concentrate on fixing them instead of diving into the Globals Are Evil discussion.
The first thing we're going to do is collect all the error counter functions into a single module, ErrCount.c, which would contain error counting, error reporting, and so on. This gets rid of the need to define g_ErrCount in globals.c, giving the below. We've also changed to using ErrCount.h for the extern definition:
globals.c: // not needed any more for this variable
ErrCount.c: int g_ErrCount; // define the variable
ErrCount.h: extern int g_ErrCount; // other files include this
init.c: g_ErrCount = 0; // init when program starts
moduleX.c: g_ErrCount++; // tally another error
moduleY.c: XVar = g_ErrCount; // get current number of errors
moduleZ.c: g_ErrCount = 0; // clear number of reported errors
Now let's get rid of the initialization. Having a central init.c is asking for problems if you forget to call an initialization function. Also, having a separate init.c forces variables to be global. So let's initialize the variable where it is defined:
globals.c: // not needed any more for this variable
ErrCount.c: int g_ErrCount = 0; // define and init variable
ErrCount.h: extern int g_ErrCount; // other files include this
init.c: // no longer needed
moduleX.c: g_ErrCount++; // tally another error
moduleY.c: XVar = g_ErrCount; // get current number of errors
moduleZ.c: g_ErrCount = 0; // clear number of reported errors
Instead of having the variable be global, let's hide it as a static variable inside ErrCount.c. Using the "static" keyword in defining a variable outside a function makes it invisible to other .c files. This step results in the program being broken, because other .c files can't get at the static variable. (We've also renamed the variable without the "g_" prefix because it's not global any more.)
ErrCount.c: static int ErrCount = 0; // only visible in this file
ErrCount.h: // static variables are invisible outside .c file
moduleX.c: g_ErrCount++; // tally another error
moduleY.c: XVar = g_ErrCount; // get current number of errors
moduleZ.c: g_ErrCount = 0; // clear number of reported errors
To fix the problem with .c files seeing the static variable, we're going to add some access functions to ErrCount.c to provide the ability to touch the value without making the variable global.:
ErrCount.c: static int ErrCount = 0; // only visible in this file
inline void ErrCount_Incr() { ErrCount++; }
inline int ErrCount_Get() { return(ErrCount); }
inline void ErrCount_Reset() { ErrCount = 0; }
ErrCount.h: inline void ErrCount_Incr(); // increment the count
inline int ErrCount_Get(); // get current count value
inline void ErrCount_Reset(); // reset count
// Note that there is NO access to ErrCount directly
moduleX.c: ErrCount_Incr(); // tally another error
moduleY.c: XVar = ErrCount_Get(); // get current number of errors
moduleZ.c: ErrCount_Reset(); // clear number of reported errors
And that's it -- we're there. ErrCount is no longer a global variable. It is visible only inside ErrCount.c, and any accesses to the variable are performed via access functions that increment, read, and reset the value. Note that the keyword "inline" should, with a good compiler, make this code just as fast and efficient as the global variable version of the code -- except without actually having a global variable. In fact, what we've been doing is a C-based approach for making ErrCount into an object (the variable) with access methods to increment, read, and reset the object. Not quite as clean as you might see in C++, but it gets the job done with C syntax.
Some folks might just say this is slight of hand. If it generates the same code, why bother? Here are some reasons that at least some developers find it useful to take this approach:
- Software authors can only perform intended functions specific to an error counter: increment, read, and reset. Setting to an arbitrary value isn't allowed. If you don't want the value changed other than via incrementing, you can just delete the reset function. This prevents some types of bugs from ever happening.
- If you need to change the data type or representation of the counter used that all happens inside ErrCount.c with no effect on the rest of the code. For example, if you find a bug with error counts overflowing, it is a lot easier to fix that in one place than every place that increments the counter!
- If you are debugging with a breakpoint debugger it is easier to know when the variable has been modified, because you can get rid of the "inline" keywords and put a breakpoint in the access functions. Otherwise, you need watchpoints, which aren't always available.
- If different tasks in a multitasking system need to access the variable, then it is a lot easier to get the concurrency management right inside a few access functions than to remember to get it right everywhere the variable is read or written (get it right once, use those functions over and over). Don't forget to make the variable volatile and disable interrupts when accessing it if concurrency is an issue.
I'm sure different readers have different ways of approaching this problem, And some globals are harder to get rid of than others. But I've seen a lot of code that is structured just like the "before" code. (I'm sure I must have written things that way myself in my mis-spent youth!) This approach cleans up a large fraction of globals with minimal pain and often no speed penalty.
Thanks for the article. I like following your blog. Actually, I instantly thought of a object-oriented solution in C++ but it's also nice to see how to properly solve this in plain C.
ReplyDeleteActually, inlining won't help if you do separate compilation of each .c file; as the inliner can only access the C code that its actively compiling.
ReplyDeleteYou'd need to have a compiler that allows reading many C files in a single compilation step.
However, even if the result is bigger code, its more robust. And robust matter more than just about anything else in embedded systems.
You're right about this. I've seen embedded systems that support inline across an entire project. Some don't, so that's something to ask your compiler vendor about. You're also right that in embedded systems having fast but buggy code isn't the right way to play the optimization game.
ReplyDeleteThx Phil for your solution. Just starters like us read something like "Global are evil" and also get the explanation but never the solution but you offer solutions which I really appreciate. Btw your book just arrived today.
ReplyDeleteDue to possible inefficiency of inlining mentioned above, I'd rather use NDEBUG switch to select between access by macros (production code) and normal functions (development code):
ReplyDeleteErrCount.c:
#ifdef NDEBUG
int ErrCount = 0;
#else
static int ErrCount = 0;
void ErrCount_Incr() { ErrCount++; }
int ErrCount_Get() { return (ErrCount); }
void ErrCount_Reset() { ErrCount = 0; }
#endif
ErrCount.h:
#ifdef NDEBUG
#define ErrCount_Incr() do { ErrCount++; } while (0)
#define ErrCount_Get() (ErrCount)
#define ErrCount_Reset() do { ErrCount = 0; } while (0)
extern int ErrCount;
#else
void ErrCount_Incr();
int ErrCount_Get();
void ErrCount_Reset();
#endif
moduleX.c: ErrCount_Incr();
moduleY.c: XVar = ErrCount_Get();
moduleZ.c: ErrCount_Reset();
I do NOT recommend this approach if you can find a way to use the inline approach. You are changing the debug code from the production code, and you are using macros. And this is complicated enough it's harder to tell if there is a bug. Adding an apparently null do/while around a single execution is not making your code easier to understand. Most compilers will do just fine with the inline keyword. If your compiler doesn't handle that properly you should look into getting a better compiler. (Yes, I know, there are always projects that are under-resourced in so many ways. Those projects also have an elevated probability of failure.)
ReplyDeleteHi Phil, I develop in C++ and typically I organize the application into a few components such as 'display', 'settings', 'logging', and top-level control is typically a stateMachine 'mainController'. All events are sent to the mainController, who then (depending on its state) dispatches to the subsytems by calling their memberfunctions.
ReplyDeleteSo at the top-level I have one instance of these objects, and I prefer to have these as 'global'. Alternatives are to have them on the stack or heap, but I don't like this :
* the object may have a large data-footprint (eg buffer for a display)
* I know I will have exactly 1 instance (objects are made nonCopyable)
* Even if the objects are global, they are properly encapsulated and all internals are private, only an API is exposed
Do you consider this use of global objects a case of using global variables ? (and as such a practice to be avoided), If so, how would you recommend to do it better ?
Thanks,
Pascal
I can't provide individual advice here, but some general observations:
ReplyDelete- Remember that static allocation is only one of two salient aspects of globals; you can get it while restricting variable scope. You can make the storage file static in main.c or even static in main and pass pointers down to the functions if that makes sense.
- You can make each of these components their own object with accessor functions. The accessor functions might have global scope, but not the actual data structures. sometimes this makes sense.
- Sometimes having a couple system-level info data structures that are globally visible can make sense, but only if other alternatives don't work.
In your case it sounds like you might be conflating "global" with "statically allocated" so it might be helpful to tease those concepts apart in your design.
Ok. Now imagine dual-core MCU with an SMP RTOS without hardware-supported atomic operations and the global context has over a thousand of variables - e.g. system registry... Having one mutex for all the context will be too expensive and can render the system literally one-core, having mutex for each of the variables is not an option, making it local is neither - do you have any recomendations in this case?
ReplyDeleteCheers,
Alan
Systems I have seen treat this as a database that handles a stream of transactions. At some point that database can still form a bottleneck but it allows you to use well understood principles to maximize performance.
Delete