Arduino fails to optimize out unreachable Serial library code

[EtherFidelity]

Many libraries use Serial output in debugging routines, and often times the serial interface is used for any number of other purposes.

Sometimes though, in a finished device, the serial interface is not used at all. In these instances, those serial output routines are never called, yet the Serial library wastes precious Flash space and SRAM.

Here's an empty Arduino project, and its compilation output:

void setup() {
  // put your setup code here, to run once:
}

void loop() {
  // put your main code here, to run repeatedly:
}

Sketch uses 450 bytes (1%) of program storage space. Maximum is 32,256 bytes.
Global variables use 9 bytes (0%) of dynamic memory, leaving 2,039 bytes for local variables. Maximum is 2,048 bytes.

Now the same code again with a simple, but unused function added:

void setup() {
  // put your setup code here, to run once:
}

void loop() {
  // put your main code here, to run repeatedly:
}

void unusedFunction() {
  Serial.println();
}

Sketch uses 1,388 bytes (4%) of program storage space. Maximum is 32,256 bytes.
Global variables use 182 bytes (8%) of dynamic memory, leaving 1,866 bytes for local variables. Maximum is 2,048 bytes.

As you can see, 173 bytes or SRAM and 938 bytes of Flash are now used that should have been available to the programmer. The SRAM is of the most concern... That's almost 17% of the SRAM available on 1k chips!

And to trigger this bug, all you have to do is have a stray Serial library reference in and file you're including (or anything it includes) and goodbye 173 bytes of SRAM.

[cousteaulecommandant]

Well, the compiler doesn't really know whether that unusedFunction() will be used on the program or not; maybe you want to use the generated ELF later as a library, so all non-static functions are considered to be reachable.

Option 1: fix your code to tell the compiler that all "global" variables and functions other than setup and loop are not meant to be used outside of this file, by using the static keyword:

static void unusedFunction() {
  Serial.println();
}

Option 2: fix Arduino so that it tells the linker to remove unused functions and variables by adding the compiler flags -ffunction-sections -fdata-sections -Wl,--gc-sections. I think this solution makes more sense as it reduces the binary size at no cost plus it doesn't involve teaching non-experts about static, so probably this should be default.

PS: I think these flags are not used. Maybe I'm wrong and Arduino already uses them, in which case I have no idea why this would happen. Still, option 1 would probably show some improvements.

[matthijskooijman]

@cousteaulecommandant, the flags you indicate in option 2 are already used, and, I think, the unusedFunction() will be removed from the final compilation as expected.

However, the problem is that the function cannot be removed until link time (unless, as you suggested you add static). Before the linker can remove any unused functions, it must resolve all open dependencies. In this case, it means that the Serial object / println function is resolved. Due to the way the linking works, this pulls in the entire source file that defines the Serial object. This is a good thing, since this also pulls in the serial interrupt routines (ISRs, which wouldn't otherwise get linked in, since they are never directly referenced). However, to prevent these ISRs from being optimized away, they are marked with the used attribute. Without that, the ISRs would be removed by --gc-sections for not being used. However, now that the ISRs are forcibly kept in the link, anything they reference is also kept in, which I believe is what you're seeing in your example.

Note that I expect that only things referenced by the ISR are kept, so if you actually call unusedFunction(), you will get a lot more code size, consisting of things which are now optimized away.

Fixing this is probably impossible or tricky. While writing this I realized that --gc-sections works on sections, not individual functions, so some trickery could be applied that allows putting ISRs in the same section as some function that enables them or something (so the used attribute is no longer necessary), but I'm not quite sure if that will work out at all (probably not, I suspect).

[cousteaulecommandant]

Once upon a time, there used to be a deprecated and now unsupported (I think) option, -combine, which told the compiler to treat all sources as a single one (this could probably still be emulated by #including every .cpp on a single file and compiling that), and then you could use -fwhole-program to tell the compiler that unused functions can be removed, just as if they were static. In other words, this trickery was a replacement for proper usage of static.
This option is not supported in new versions of GCC I think, and there probably are good reasons for that, so doing this is probably a bad idea anyway.
(No idea how this will work for compiling mixed C and C++; probably not well)

[matthijskooijman]

@cousteaulecommandant, nowadays, there is link-time-optimization (LTO) which can be used to achieve pretty much the same effect I believe. However, that doesn't solve this particular problem, since the compiler still doesn't know when to (not) include ISRs.

[PaulStoffregen]

Every time Arduino's use of LTO comes up, problems with the USB Host Shield library seem to come up...

[cousteaulecommandant]

@matthijskooijman, that was my point; LTO doesn't deal with this problem correctly. Then again, maybe -fwhole-program doesn't either since interrupts are marked as externally_visible, so maybe using this trick ends up including ALL the interrupts, even the ones not referenced. Forget this idea.

The best solution would be to tell GCC to mark all function and object definitions in a file as static except for setup and loop. I think this could be achieved by using -fwhole-program when compiling the main Arduino source file and marking setup and loop as externally_visible. (I made some experiments with plain gcc -c and a C file and it seems to work; next step would be trying this with Arduino.)

(Then again, is this really needed? Isn't the user expected to just not define unused functions?)

[cousteaulecommandant]

And to trigger this bug, all you have to do is have a stray Serial library reference in and file you're including (or anything it includes)

If you're worried about someone making a utility library where some functions use Serial (for example), then I think the rules change, since (I think) only symbols needed from an object file will cause the inclusion of other object files, so if I understood correctly the object file containing Serial will not be pulled if you don't use any of the functions from that library that use Serial.
Additionally, the library could (should?) be split into multiple files so that only the ones with functions you're using get pulled.

[matthijskooijman]

@matthijskooijman, that was my point; LTO doesn't deal with this problem correctly. Then again, maybe -fwhole-program doesn't either since interrupts are marked as externally_visible, so maybe using this trick ends up including ALL the interrupts, even the ones not referenced. Forget this idea.

The externally_visible / used attribute on the ISRs will indeed prevent -fwhole-program from working here (unless -fwhole-program ignores that attribute, but then ISRs will always be optimized away, which isn't quite what you need either.

I don't think -fwhole-program will really work for what you need here, unless the compilation is changed to compile everything in one gcc call (instead of separate compile and link steps as now). To make things work now, you would need smarter handling of ISRs. Ideally, you would be able to declare a dependency from all functions that enable an ISR to the ISR function itself, for the linker to see (but I'm not sure if this is possible in a way that does not generate any runtime code). Then you could remove the used attribute from the ISR. Alternatively, the ISR could be put in a single compile unit with (only) functions that enable it, and keep the used attribute. This might work, though it will still break if one of those functions is pulled in during link and later optimized out by gc-sections again.

Additionally, the library could (should?) be split into multiple files so that only the ones with functions you're using get pulled.

This is exactly why C libraries used to do a 1-function-per-file approach in the old days (now, -ffunction-sections and -fgc-sections remove the need for this, except perhaps for ISRs and other functions that are marked as externally_visible / used and do not play by the normal rules).

[cousteaulecommandant]

Just to clarify, I meant to use -fwhole-program on the source file generated from the .ino files, not the rest of the Arduino library; the idea was to "add static to everything written by the user" automatically, instead of making the user type static on their code.

According to the GCC documentation:

-fwhole-program
[...] All public functions and variables with
the exception of "main" and those merged by attribute
"externally_visible" become static functions and in effect are
optimized more aggressively by interprocedural optimizers.
[...] While this option is equivalent to proper use
of the "static" keyword for programs consisting of a single file, [...]

So, if I understood correctly, -fwhole-program is equivalent to using static (and some tests I made showed that this option takes effect at compile time rather than at link time). And since using static seemed to solve the problem, I guess this would too. I didn't figure out how to set Arduino to compile the user program (.ino) with different options from the rest of the Arduino library though (and I was too lazy to run all the commands manually), so I couldn't confirm this works, but I'd say it probably does.

[matthijskooijman]

Just to clarify, I meant to use -fwhole-program on the source file generated from the .ino files, not the rest of the Arduino library; the idea was to "add static to everything written by the user" automatically, instead of making the user type static on their code.

Ah, I see now. However, this approach actually changes the meaning of the code. For symbols that are only used inside the .ino file where the user simply forgot to add static (which is most of them), this shouldn't matter, but for other symbols which are (weakly) referenced by external libraries, this approach will break (e.g. consider serialEvent() which is defined by the sketch but called by the Arduino core).

[matthijskooijman]

I think this is something we cannot really fix, since it is just a limitation of the C/C++ language. In particular, it is pretty much impossible for the compiler to tell whether an ISR is actually enabled or not. We already support leaving out the Serial ISR (and related code) if you do not reference the Serial object at all, which I think is the best we can do.

If anyone has any ideas on how to fix this, I'm happy to hear them. Until then, I'm closing this issue.

[MCUdude]

@matthijskooijman sorry for bringing this one up after so long, but I've been trying to figure out the mechanism for optimizing out unused Serial ISRs. Where in the code or build system is this happening? I'm asking because I'd like to do the same in a library; leave out the ISR if it's not being used.

Thanks!

[MCUdude]

For reference, the library I'm trying to modify is the Logic library A wapper for the CCL hardware for the megaAVR-0 series. The problem is that ISR(CCL_CCL_vect) is present in Logic.cpp, and is not optimized out even if the ISR is not in use. I've tried to move the ISR into its own .cpp file, but I still can't get the compiler to optimize it away

[matthijskooijman]

@MCUdude, for some background on how this was implemented for HardwareSerial, see arduino/Arduino#1711 (comment) and for a similar thing for the timer ISR, see #320

One important bit here, is that this trick only works when linking through .a files. This is the default for the core, but libraries are linked through .o files by default (meaning each .o file of the library is specified explicitly on the linker commandline, and thus is included in the link unconditionally, and then the used attribute on the ISR prevents it from being optimized away again). You can change this using a value in library.properties (it's called dot_a_linkage I think, or something similar), which causes the library's .o files to be collected in a .a file, and that causes the linker to only include .o files that are actually needed to satisfy a missing symbol.

Hope this helps. If you have further questions, maybe good to open an issue in your library for further discussion? If you tag me, I'll find the issue from the notification :-)

[MCUdude]

Thank you for your reply @matthijskooijman!
I ended up moving the ISR into its own cpp file, and with dot_a_linkage=true, it's being optimized out!