The other day I was checking out the repository for something when I stumbled upon a goto statement. I didn’t pay any attention to it at the time as I was focusing on something else. But it kinda stuck with me. Today I was decided to check out the repository to see how many times it’s used and it seems like we’re using it pretty frequently.
My question is “Why?!” Back in university my intro to coding professor drilled into my head that it’s usage was an anti-pattern and should not be used unless absolutely necessary. And since then I didn’t find any use for it except breaking out of double-nested loops or certain error conditions. But it seems like we’re not using it for these. We even use it for jumping to locations where we just call a function and exit if an error condition is detected. Why not just call the function directly and return?
Is there a particular reason for using them or it just happened and we kept it that way. If it’s the latter, I’m inclined to go ahead and remove them as dump.c looks particularly juicy to me.
I also have a strong “anti-goto” background. I try not to use it. However, there is currently no hard requirement to avoid its use in the project. I would definitely avoid goto in any C++ code, and at the same time, there are limited patterns where a goto has made a function easier to read.
I’ll follow-up with another post with some examples (need time to write them), both good and bad.
All that said, if there is working code now, I have learned many hard lessons on just how easy it is to introduce bugs with “simple” refactorings. As a result, I generally recommend against changing code manually for the sake of coding style changes.
int foo(void) {
int a = 10;
for (int i = 0; i < 30; ++i) {
int x = 17;
prepare_outer_loop(i, &a);
repeat_outer:
for (int j = 0; j < 40; ++j) {
int y = 31;
do_some_work(i, j, &a);
if (check_some_condition()) {
goto repeat_outer;
}
}
}
return a;
}
This shows an example where the flow of the code’s execution is difficult to follow because of the use of goto. Maybe it’s the best way, but more likely it suggests a problem with how the code is factored.
Example of terrible usage
What is this code guaranteed to do?
int main() {
goto do_the_work;
// ...
int x = 127;
// ... and then some lexical scoping from if/while/do/whatever
{
int i=10;
do_the_work:
printf("%d, %d\n", i, x);
}
if ((i != 10) || (x != 127)) {
goto do_the_work;
}
return 0;
}
Answer: Undefined behavior. Neither i nor x is ever initialized. And yes, this is legal in C, even if C++ would reject it.
Generally, I will only intentionally plan to use goto in one scenario:
Ensuring that all function-local resources are properly cleaned up.
Let’s walk through three examples, starting at the common obvious pattern, moving to a single function exit point, and finally to the one of use goto that I grudgingly do not complain about.
1. Example: common pattern
This is a common pattern, especially for early-in-career coders:
int foo(void) {
void* alloc_a = malloc( 1024 * 3 );
if (!alloc_a) {
return E_ALLOCATION_FAILURE;
}
// do some work with alloc_a
void* alloc_b = malloc( 1024 * 7 );
if (!alloc_b) {
free(alloc_a);
return E_ALLOCATION_FAILURE;
}
// do some work with alloc_b
// resource_t is an opaque structure
resource_t resource;
if (!allocate_resource(&resource)) {
free(alloc_b);
free(alloc_a);
return E_ALLOCATION_FAILURE;
}
// do work with all three resources...
release_resource(&resource);
free(alloc_b);
free(alloc_a);
return E_SUCCESS;
}
Note that the work done with alloc_a and alloc_b early on are independent. If someone later swaps the order of allocation of those two, they also have to run through all the exit points to ensure things are free’d / released accordingly.
Not shown, but also relevant:
Any failure points inside the “work” comments also have to free exactly the right set of resources … and thus be edited if the order of resource allocation / work is changed. This increases the likelihood of bugs being added during maintenance work.
There’s also many, many exit points in that function. This makes it harder to ensure that the code will always cleanup all local resources, and setting a breakpoint on the return from this function may require many breakpoints. No problem on x86/x64, but most IoT class devices have a very limited number of hardware breakpoints.
2. Example: Single function exit using success flag
This is a “good” pattern for maintainable code. I understand that some folks dislike the wrapping of all “real” work in if blocks. That’s valid … and addressed in next example.
All resource-storing variables are declared at the top of the function.
Tracking which of those variables needs cleanup is done either via null or other invalid value, or using a separate boolean to track initialization. Finally, the return result is also declared at the top of the function.
Because it has a single function exit point:
a single hardware instruction breakpoint covers all function exit points
a single hardware break-on-write for result catches all errors
easy to add trace / debug output of function result (only one exit point)
easy for maintainers to see what is allocated by function
easy for maintainers to verify that all resources are free’d at end of function
the order of resource allocation can change without introducing leaks (c.f. prior pattern with lots of lists of resource free’ing throughout the function)
this pattern encourages later maintainer who adds a new resource to follow the same pattern
optimized?
failure paths are not performance critical
even so, compiler can optimize failure paths to jump to the cleanup section directly (bypassing intermediate checks of if (SUCCESS(result)))
int foo(void) {
void* alloc_a = nullptr;
void* alloc_b = nullptr;
resource_t resource; // opaque structure
bool resource_initialized = false;
int result = E_SUCCESS;
if (SUCCESS(result)) {
alloc_a = malloc( 1024 * 3 );
if (!alloc_a) {
result = E_ALLOCATION_FAILURE;
}
}
if (SUCCESS(result)) {
// do some work with alloc_a
}
if (SUCCESS(result)) {
alloc_b = malloc( 1024 * 7 );
if (!alloc_b) {
result = E_ALLOCATION_FAILURE;
}
}
if (SUCCESS(result)) {
// do some work with alloc_b
}
// following function returns true on success
if (SUCCESS(result)) {
resource_initialized = allocate_resource(&resource);
if (!resource_initialized) {
result = E_ALLOCATION_FAILURE;
}
}
if (SUCCESS(result)) {
// do work with all three resources...
}
// cleanup all resources
if (resource_initialized) {
release_resource(&resource);
}
if (alloc_b != nullptr) {
free(b);
}
if (alloc_a != nullptr) {
free(a);
}
return result;
}
3. Example: cleanup using goto
Some folks, because the above if (SUCCESS(result) blocks grate on them significantly, would prefer to use goto … seeing it as a “lesser evil”. This is entirely a personal preference. In C, this is functionally equivalent to the “success flag” option.
Some folks also prefer this because it makes the goto act substantially like a throw, with the label acting similar to a catch.
The code is less indented. It’s also arguably easier to read. The same benefits are achieved as the prior example.
int foo(void) {
void* alloc_a = nullptr;
void* alloc_b = nullptr;
resource_t resource; // opaque structure
bool resource_initialized = false;
int result = E_GENERIC_ERROR;
alloc_a = malloc( 1024 * 3 );
if (!alloc_a) { goto cleanup; }
// do some work with alloc_a
alloc_b = malloc( 1024 * 7 );
if (!alloc_b) { goto cleanup; }
// do some work with alloc_b
// following function returns true on success
resource_initialized = allocate_resource(&resource);
if (!resource_initialized) { goto cleanup; }
// do work with all three resources...
result = E_SUCCESS;
cleanup:
if (resource_initialized) {
release_resource(&resource);
}
if (alloc_b != nullptr) {
free(b); b = nullptr;
}
if (alloc_a != nullptr) {
free(a); a = nullptr;
}
return result;
}
Thus, that third pattern is the one way I would approve of use of goto. It also encourages other patterns that are helpful for debugging on resource-constrained hardware (e.g., IoT class ARM CPU; c.f. x64 CPU).
Even where most hardware hackers are proficient in coding, there may be a focus is on “getting something to work”. My background has more exposure to ensuring long-term maintainability. This includes code maintainance by folks other than the original author.
It’s Paul’s fault I don’t think I had ever used goto in C, but Paul used it heavily in the scope. Paul is the most experienced embedded developer I have ever met in person (like, from the very very beginnings). So I started using it here and there.
It is used especially in commands that have a lot of “if error we have to clean up”. It consolidated the cleanup into one place. The other options being to use a function which as I understand hits the stack (vibes based), or inline it in which case one I2C chip demo takes 16*calls extra flash.
I especially used it when adding safeguards to code that was giving people issues. Cause it was a lot and I’m sometimes lazy.
I won’t defend my offensive actions feel free to do whatever and PR it, to dump or elsewhere.
Thanks @henrygab , those were really good examples. And I totally agree with
This is mainly what I’m worried about. The code works now and I’m fairly sure some cases can be “fixed” easily. Like dump.c file that I mentioned earlier.
Current code
void dump_handler(struct command_result* res) {
// we can use the ui_help_show function to display the help text we configured above
if (ui_help_show(res->help_flag, usage, count_of(usage), &options[0], count_of(options))) {
return;
}
// get number of bytes to read argument
uint32_t dump_len = 0;
bool has_len = cmdln_args_uint32_by_position(1, &dump_len);
if (!has_len) {
printf("Error: No length specified\r\n\r\n");
goto display_help;
}
// get filename argument
char filename[13];
if (!cmdln_args_string_by_position(2, sizeof(filename), filename)) {
printf("Error: No filename specified\r\n\r\n");
goto display_help;
}
........ Bunch of lines not really relevant ........
// close the file
f_close(&file);
return;
display_help:
ui_help_show(true, usage, count_of(usage), &options[0], count_of(options));
}
can be changed to something like
void dump_handler(struct command_result* res) {
// we can use the ui_help_show function to display the help text we configured above
if (ui_help_show(res->help_flag, usage, count_of(usage), &options[0], count_of(options))) {
return;
}
// get number of bytes to read argument
uint32_t dump_len = 0;
bool has_len = cmdln_args_uint32_by_position(1, &dump_len);
if (!has_len) {
printf("Error: No length specified\r\n\r\n");
ui_help_show(true, usage, count_of(usage), &options[0], count_of(options));
return;
}
// get filename argument
char filename[13];
if (!cmdln_args_string_by_position(2, sizeof(filename), filename)) {
printf("Error: No filename specified\r\n\r\n");
ui_help_show(true, usage, count_of(usage), &options[0], count_of(options));
return;
}
........ Bunch of lines not really relevant ........
// close the file
f_close(&file);
return;
}
What I’m wondering:
Is there a particular reason for some of these? (I guess @ian answered this question. Kinda. )
Should we spend any effort in “fixing” these? Will it improve readability or any potential issues?
If we decide to “fix” them, how can we easily test for some weird behavior these changes may cause?
Sorry if my original message came as “Why are we doing this totally wrong thing! We need to change this NOW!!!” I’ll blame it being pretty late over here when I originally wrote it.
There is no reason not to make the changes you show above. It does repeat a call with 5 variables which must take a bunch more instructions than a single jump to line instruction. Not that we’re trying too hard to save flash space, but that is my general thinking.
The goto version is arguably more maintainable, as any change to the help function only needs to be updated in one or two places.
I don’t think it’s particularly defensible to use goto in dump. There are other places where a lot more stuff would have to be repeated.
goto madness
goto flash_cleanup;
} else {
goto flash_cleanup;
}
if (erase || erase_flag || test) {
if (!spiflash_erase(&flash_info)) {
goto flash_cleanup;
}
if (verify_flag || test) {
if (!spiflash_erase_verify(start_address, end_address, sizeof(data), data, &flash_info)) {
goto flash_cleanup;
}
}
}
if (test) {
if (!spiflash_write_test(start_address, end_address, sizeof(data), data, &flash_info)) {
goto flash_cleanup;
}
if (!spiflash_write_verify(start_address, end_address, sizeof(data), data, &flash_info)) {
goto flash_cleanup;
}
}
if (write) {
if (!spiflash_load(start_address, end_address, sizeof(data), data, &flash_info, file)) {
goto flash_cleanup;
}
if (verify_flag) {
if (!spiflash_verify(start_address, end_address, sizeof(data), data, data2, &flash_info, file)) {
goto flash_cleanup;
}
}
}
if (read) {
if (!spiflash_dump(start_address, end_address, sizeof(data), data, &flash_info, file)) {
goto flash_cleanup;
}
}
if (verify) {
if (!spiflash_verify(start_address, end_address, sizeof(data), data, data2, &flash_info, file)) {
goto flash_cleanup;
}
}
flash_cleanup:
//we manually control any FALA capture
fala_stop_hook();
fala_notify_hook();
}
This isn’t the best example, but it is one I have open right now. In flash.c there are a bunch of things that we might need to bail on.
fala_stop_hook();
fala_notify_hook();
return;
Before we bail we need to stop the follow along logic analyzer, so we’d need to add three lines to every instance.
no madness
fala_stop_hook();
fala_notify_hook();
return;
} else {
fala_stop_hook();
fala_notify_hook();
return;
}
if (erase || erase_flag || test) {
if (!spiflash_erase(&flash_info)) {
fala_stop_hook();
fala_notify_hook();
return;
}
if (verify_flag || test) {
if (!spiflash_erase_verify(start_address, end_address, sizeof(data), data, &flash_info)) {
fala_stop_hook();
fala_notify_hook();
return;
}
}
}
if (test) {
if (!spiflash_write_test(start_address, end_address, sizeof(data), data, &flash_info)) {
fala_stop_hook();
fala_notify_hook();
return;
}
if (!spiflash_write_verify(start_address, end_address, sizeof(data), data, &flash_info)) {
fala_stop_hook();
fala_notify_hook();
return;
}
}
if (write) {
if (!spiflash_load(start_address, end_address, sizeof(data), data, &flash_info, file)) {
fala_stop_hook();
fala_notify_hook();
return;
}
if (verify_flag) {
if (!spiflash_verify(start_address, end_address, sizeof(data), data, data2, &flash_info, file)) {
fala_stop_hook();
fala_notify_hook();
return;
}
}
}
if (read) {
if (!spiflash_dump(start_address, end_address, sizeof(data), data, &flash_info, file)) {
fala_stop_hook();
fala_notify_hook();
return;
}
}
if (verify) {
if (!spiflash_verify(start_address, end_address, sizeof(data), data, data2, &flash_info, file)) {
fala_stop_hook();
fala_notify_hook();
return;
}
}
Here is the same code refactored not to use goto. It is substantially longer, and it would be a pain to add a new cleanup step to each line.
We could call a function or an inline function to make it a more maintainable/cleaner. Then we still have 2 lines instead of 1 because we’d need to return after the new function/inline function call.
My understanding is that learning programmers are discouraged from using goto for several reasons, but it’s more akin to starting a kid on a tricycle instead of a Harley Davidson crotch rocket.
I have worked with a lot of barely competent web developers, and there is no way on earth I’d let them use the equivalent of goto in one of my projects. Once a “10 year senior e-commerce developer” gave me a shopping cart system that stored the prices of goods in hidden form fields in the users browser. The checkout then accepted those prices without any validation they had not been manipulated on the user side. I demoed and explained what “F12” does and how that was a huge security flaw and they just couldn’t or wouldn’t accept it. This type would happily bring down the whole website with an errant goto to nowhere or in a never ending loop.
That said, there are similar vulnerabilities with loops:
for(uint8_t i=0; i<257; i++)
while(true){
do{}until()
Instead of banishing loops we teach their limitations and gottchas.
100 if x = 1 goto 103
101 print "x is not one"
102 exit
103 do the x = 1 business
Another reason older programmers may think GOTOs are the ultimate sin could be BASIC. My first language was Apple BASIC. Function-like behavior involved GOTO <line number>. When I graduated to a big boy language with actual function calls it was like a whole different world full of color and texture. Who would ever want to go back to that swamp?
ASM developers, however, never left that swamp. It is perfectly valid and necessary ASM to use JMP instructions (the underlying instruction for GOTO). I’m far from an expert, but I have done several dozen PIO ASM programs and JMP is just part of life.
My general impression (not opinion) is that goto is a perfectly fine command if:
You are aware of how it can create a indecipherable mess to work with
You are aware of the vulnerabilities to get stuck in loops and other side effects
You have an awareness of why you use goto certain places instead of a function call
I believe in our codebase goto is usually used as a “drain”. We have a dozen or more if ok continue, if not cleanup and exit type situations in some commands. If it errors out we just goto the end, cleanup and exit.
That’s all a lot to say:
I didn’t get any sense of offense from your late night message
I’m not hardcore on the gotos, and would happy remove them
I don’t think they are dangerous as currently used as a final command exit point, but I don’t mind if something else more “proper” is used
You might think I have a strong opinion after writing so much, but I really don’t
As a final command exit “drain” that does repetitive cleanup steps in a single place.
Both, in different cases. dump - sure! it isn’t that different. flash - it would expand the code length greatly which would decrease maintainability and readability.
I don’t know on this one. Any changes need to be tested thoroughly and we don’t currently have any unit testing or automated testing.
The dump function could be structurally improved. This is true of most of the command parsing functions. Imagine if the code was commonly refactored to:
/* ... */ {
if (!validate_arguments(...)) {
show_help(...);
return E_FAILURE;
}
// declare and initialize all the "resources"
int result = E_FAILURE;
// do the work, use `goto cleanup_resources;` on error
// everything worked if got here...
result = E_SUCCESS;
cleanup_resources:
// cleanup resources in reverse order
return result;
}
The above is a strong design pattern for C. It reduces common errors. I had seen this pattern in the Windows kernel mode device drivers (also written in C). Even the Linux folks like this design pattern.
The biggest difference is that, currently, the function does not segment the paramater validation into its own scope, resulting in an extra goto label. Even when parameter validation is not a separate function, using a if (true) {} block (or just {}) to wrap the parameter validation helps later maintainers logically group the validation, which reduces the programmer’s mental load.
By separating out (as much as possible) failure points prior to the actual “work” (including allocation of resources), it greatly reduces the failure paths that leave things half-done.
I had to program for about 6 years before our company found a portable language that supported structured programming. It was either assembler, FORTRAN or BASIC. GOTO’s and subroutines were the only choices, other than
GOTO is trivial to implement. It’s a simple machine instruction. We have machines that had total 4K or memory. BASIC ran in 4K with room to spare. So the first home computers were severely limited in memory, and sophisticated compilers were not available.
So, many programmers (especially the older ones), grew up with BASIC and GOTO’s. In my experience, it takes at least a year of solid programming to really begin to think in the new language. Until then, you solve problems using the same logic structure of your last language.
I don’t think I had ever used goto in C, but Paul used it heavily in the scope. Paul is the most experienced embedded developer I have ever met in person (like, from the very very beginnings). So I started using it here and there.
feel free to do whatever and PR it, to dump or elsewhere.
