lib: cbprintf: add support for deferred formatting #32734
Conversation
npitre
requested review from
MaureenHelm,
andyross,
carlescufi,
dcpleung,
galak and
nashif
as code owners
github-actions
bot
added
area: API
lib/os/cbprintf_packaged.c
Outdated
There was a problem hiding this comment.
you have to store float argument even though it is not supported, otherwise output is corrupted for strings containing %f
There was a problem hiding this comment.
more precisely, you have to consume the float argument even if you're going to discard it.
lib/os/cbprintf_packaged.c
Outdated
There was a problem hiding this comment.
i would prefer that this is stored as u16 together with the value above which also could be u16. 64k should be more than enough for both values and it results in smaller and aligned buffer in the typical cases.
lib/os/cbprintf_packaged.c
Outdated
There was a problem hiding this comment.
what about packaged buffer alignment. It does not matter when cbpprintf is called from the same memory but if package is copied then new buffer must have same alignment as the original one. Maybe require buffer to be aligned?
lib/os/cbprintf_packaged.c
Outdated
There was a problem hiding this comment.
The l and ll length specifiers mean something different for integral, character, and floating point types.
lib/os/cbprintf_packaged.c
Outdated
There was a problem hiding this comment.
more precisely, you have to consume the float argument even if you're going to discard it.
|
On Mon, 1 Mar 2021, Peter A. Bigot wrote:
The `l` and `ll` length specifiers mean something different for integral, character, and floating point types.
`%lc` is already covered. I don't think we support `%lg` anyway?.
And what is `%llg` supposed to do? We surely don't support it either.
more precisely, you have to consume the float argument even if you're going to discard it.
Yeah. This oneis a problem because it often involves a different
register file, and those registers might be forbidden (e.g. with
-mgeneral-regs-only on Aarch64). But that's a problem for the print side
too. So I'll just consume the data for now and address that FP issue all
at once separately.
|
|
On Mon, 1 Mar 2021, Krzysztof Chruscinski wrote:
@nordic-krch commented on this pull request.
seems ok, there is a benefit of have it also for cprintf_nano. I will try to adapt static packaging.
I preserved almost the same API as Peter's. But the packaging can still
be improved in many ways as the va_list rules aren't the most space
efficient ones.
For one thing, I currently don't remove string pointers even though
they're useless as they need to be replaced on the consumer side. And on
64-bit targets there is a lot of padding inserted to align everything to
8-byte boundaries that could also be removed. Both of these could be
handled more or less with the same mechanism.
+ /*
+ * Remember end of argument list location.
+ * We made room for it earlier.
+ */
+ *(int *)(buf0 + sizeof(char *)) = buf - buf0;
+
+ /* store the number of appended strings */
+ *buf++ = s_idx;
i would prefer that this is stored as u16 together with the value above which also could be u16. 64k should be more than enough for both values and it results in smaller and aligned buffer in the typical cases.
Storing them together isn't a bad idea per se. However that will save
only one byte of storage, and have no material effect on alignment as
what follow are only strings with no alignment.
|
Strings are optional and rare case and this field is obligatory. When there are no strings, it still needs to be set to 0. Package will be used in logging messages which will be at least word aligned. Typically, a message will have 6-10 words so one additional word is significant. |
|
On Mon, 1 Mar 2021, Krzysztof Chruscinski wrote:
Strings are optional and rare case and this field is obligatory. When there are no strings, it still needs to be set to 0. Package will be used in logging messages which will be at least word aligned. Typically, a message will have 6-10 words so one additional word is significant.
OK makes sense. It helps to understand the use case.
I'll probably move the arg_size/string_count fields first. The idea with
the current location was to help with architectures where a pointer has
a greater alignment than an int, so subsequent int arguments could be
placed right after it. But so far every architectures where pointer
alignment is bigger than an int's also require that int args be aligned
to the greater alignment value so nothing is gained.
By having the counts first it will allow for discarding the string
pointers including the format string itself when those strings are
appended.
As to the earlier comment about buffer alignment: it is true that the
buffer itself must be strongly aligned, at least to the largest
alignment required by all elements in the argument list. That's usually
imposed by floats whose memory access might necessitate a strong
alignment on some platforms.
|
npitre
force-pushed
the
cbprintf_packaged
branch
from
9435f7f to
9518134
Compare
|
looks ok now, can you extract tests into separate commit? |
npitre
force-pushed
the
cbprintf_packaged
branch
from
9518134 to
863c21e
Compare
|
On Tue, 2 Mar 2021, Krzysztof Chruscinski wrote:
looks ok now, can you extract tests into separate commit?
Done.
I've also updated the documentation. The API is slightly different from
Peter's as the actual size that would have been needed is not available
when the return code is -ENOSPC. Implementing that would make the code
more complex for doing essentially the same as calling it first witha
NULL pointer. I could reconsider if that is a critical option, but I
suspect it is not.
|
No problem from my perspective. I tried to add a test that runs on all qemu's targets. I've seen two issues:
My work with static packaging and |
npitre
force-pushed
the
cbprintf_packaged
branch
from
863c21e to
b265cd3
Compare
|
I integrated your sparc workaround, fixed the xtensa case and
added your test code minus the static stuff.
|
There was a problem hiding this comment.
This seems fine.
Blocking due to an offline request from @carlescufi: For the commits that have things like:
[ Documentation and commit log shamelessly stolen from Peter Bigot. ]
[ Heavily based on a prior proposal from Peter Bigot. ]
please instead add my Signed-off-by from the original commit to track provenance in accordance with Zephyr policy.
|
On Thu, 4 Mar 2021, Peter A. Bigot wrote:
please instead add my Signed-off-by from the original commit to track provenance in accordance with [Zephyr policy](https://docs.zephyrproject.org/latest/contribute/index.html#developer-certification-of-origin-dco).
According to
https://docs.zephyrproject.org/latest/contribute/modifying_contributions.html#modifying-contributions
I did option 2. If you prefer that I simply add your sign-off that's
fine with me.
|
npitre
force-pushed
the
cbprintf_packaged
branch
from
b265cd3 to
8d134f4
Compare
@npitre thanks. If you don't mind, then I think it's clearer this way. |
|
I did option 2. If you prefer that I simply add your sign-off that's
fine with me.
Actually I did both so to give you proper credits.
|
@carlescufi If that's being interpreted to mean that incorporating material from contributions without preserving the signed-off-by line is acceptable, then I think that needs to be fixed. It has to be understood that "signed-off-by" does not mean "agrees with the content and purpose of the commit"; it means "contributions conform to DCO". IMO. IANAL. |
Looking back at the text that you and @npitre linked to, I believe you are right @pabigot. Would you mind sending a PR with a proposal to fix this in the doc? |
|
On Thu, 4 Mar 2021, Peter A. Bigot wrote:
The comment about "shamelessly stolen" seems unnecessary, but the signed-off-lines confirm legal provenance.
This is a common idiom in the Linux world in such circumstance.
Of course I may reword this if you're uncomfortable with it.
|
npitre
force-pushed
the
cbprintf_packaged
branch
from
8d134f4 to
c6de351
Compare
In applications like logging the call site where arguments to formatting are available may not be suitable for performing the formatting, e.g. when the output operation can sleep. Add API that supports capturing data that may be transient into a buffer that can be saved, and API that then produces the output later using the packaged arguments. [ Documentation and commit log from Peter Bigot. ] Signed-off-by: Nicolas Pitre <[email protected]> Signed-off-by: Peter Bigot <[email protected]>
Tests to exercise the new `cbprintf_package()`, `cbvprintf_package()` and `cbpprintf()`. [ Heavily based on a prior proposal from Peter Bigot. ] Signed-off-by: Nicolas Pitre <[email protected]> Signed-off-by: Peter Bigot <[email protected]>
Add test to validate cbprintf packaging on various platforms Signed-off-by: Krzysztof Chruscinski <[email protected]>
npitre
force-pushed
the
cbprintf_packaged
branch
from
c6de351 to
935ee55
Compare
|
Pushed a few cleanups and minor changes. @nordic-krch the two header values are now uint8_t instead of uint16_t This leaves some room (16 bits on 32-bit systems and 48 bits on 64-bit |
6 participants
This is an alternative to PR #30675.
Before adding packaged support to cbprintf_complete.c:
With packaged support added to cbprintf_complete.c per PR #30675:
This is a code growth of 2070 bytes, in addition to non negligible stack
usage as documented in that PR.
With this alternative approach here we have:
( Those numbers are for Aarch64 )
Furthermore, this is completely independent from the printing facility
meaning that it is compatible with both cbprintf_complete and
cbprintf_nano, and cbprintf_complete won't degrade due to additional
stack usage as it remains unchanged.