Print and debug improvements #1750
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Static functions are private to the compilation unit they are emitted in, so they cannot be shared between compilation units. This means that any source file that uses `core_debug()` where the compiler does not inline (all) calls, will have its own private copy of this function emitted. In practice, gcc seems to never inline this function (even with -O3), leading to one copy of the function for each compilation unit it is used in. This fixes this by removing the `static` keyword from the function. However, this prevents the function from being emitted completely in C compilation units (C++ is different and emits multiple copies, discarding all but one later). This means that if the function is only used from C compilation units and not inlined everywhere, you get a linker error. Thet `static` keyword was probably added to work around this, without realizing the overhead. The proper way to prevent this linker error is to add an `extern` definition for the function in a single source file, so this adds a `core_debug.c` with exactly that. In practice, this means that this commit saves 40 bytes of space for each compilation unit where `core_debug()` is used (beyond the first).
fpistm
requested changes
Jun 27, 2022
This does the same as core_debug, but (like printf vs vprintf) accepts an already processed va_list to allow other vararg functions to forward their argument lists to this function.
matthijskooijman
force-pushed
the
print-and-debug-improvements
branch
from
4667777
to
47c8d9f
Compare
This does the same as printf, but (like the vprintf libc function) accepts an already processed va_list to allow other vararg functions to forward their argument lists to this function.
matthijskooijman
force-pushed
the
print-and-debug-improvements
branch
from
47c8d9f
to
b014a94
Compare
|
Good call, should have tested that. Fixed (and tested ;-p) now. |
fpistm
approved these changes
Jun 29, 2022
There was a problem hiding this comment.
Thanks @matthijskooijman for this PR, it LGTM.
About the failed CI, it is not linked to this PR but to an update of the arduino-lint which raised new error/warning. This is fixed thanks #1754.
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR makes some changes to the printing/printf code, both in the Print class and in
core_debug.The main change that I needed is to add a "v" version of the
core_debug()function. Likevprintfvsprintf, thevcore_debugfunction does not accept individual arguments to interpolate into format string, but ava_listinstance. This allows it to be called from another function that is itself a varargs function and forwards its variable arguments.I originally thought I needed this for
Print::printf, so I implementedPrint::vprintftoo. I ended up usingcore_debug()instead, but thevprintf()implementation might be useful for others (and adds zero code space if unused), so I just added it.Finally, when looking at the
core_debug()code, I noticed the function wasstaticand declared in a header file, which causes it to potentially take up a lot more flash space than needed, so I fixed that as well. See the commit message for details.