lib: os: cbprintf: Add static packaging

[nordic-krch]

Alternative take to #31102, based on #32734.

Statically means that a package (serialized string with arguments) is created as assignment to a buffer. Opposed to runtime package creation with cbprintf_package() where string is analyzed and based on format specifiers a package is created (any transient strings are copied into the package buffer).

In principle, CBPRINTF_STATIC_PACKAGE(buf, &len, "test %d", n) resolves to simple assignments.

CBPRINTF_MUST_RUNTIME_PACKAGE is used to determine at compile time if static packaging can be applied.
CBPRINTF_STATIC_PACKAGE(NULL, &len, "test %d", n) - null buffer results in length calculation

Package on stack can be created with following code:

size_t len;
CBPRINTF_STATIC_PACKAGE(NULL, &len, "test %d", n);
uint8_t buf[len]; /* len is determined at compile time */
CBPRINTF_STATIC_PACKAGE(buf, &len, "test %d", n);

Note that static packaging is using _Generic C11 keyword with fallback to runtime packaging if not available and local suppressing of pointer-arith warning ((arg) + 0 generates warning if arg is void *).

[andyross]

I've been busy with other stuff and avoiding the printf drama. And FWIW this code looks good to me.

But... is this solving the wrong problem? As I understand it, the goal behind deferring formatting is mostly to get the output of the log subsystem serialized in a form that does not require formatting on the device at all, right? How much surgery to our printf layer is really appropriate given that as a goal?

I was looking at dictionary logging at one point too, and my (somewhat vague) plan was something more like:

1. Put the format string in a separate non-loadable section and give it a unique value at runtime via linker magic
2. Serialize the arguments and the ID in a recoverable way at the point of logging
3. Hand that stuff to an offline python script to figure out
4. Profit

I mean, is this code even going to be run on deployed production device? Or are we spending all this effort to come up with a scheme to allow devices to duplicate the production code in some kind of debug mode?

[nordic-krch]

@andyross packaging it's not only about dictionary logging. Current logging scheme is storing all arguments into void * items. This limits logging data (no double, long long, etc.). Additionally, for %s only string address is used and user must call log_strdup explicitly to dump the string into dedicated pool. Another aspect is multi-domain logging: log_strdup makes it harder to pass log messages between domains or copy a message. Same goes for dictionary logging or logging to flash: we would like to have self contained package that can be stored and formatted later. This is the motivation for having packaging at all. Now, static packaging is an extension to speed things up. If there are no %s in the string then package can be created statically as simple memory assignments, no need to walk through the string. With _Generic you can detect if there are any char * arguments and if not you can simply dump all arguments (utilizing _Generic to promote floats to double) and create the package. For dictionary backends, message string can be put into the section which is removed from the build and processing happens as you wrote.

[npitre]

On Thu, 4 Mar 2021, Andy Ross wrote: How much surgery to our printf layer is really appropriate given that as a goal?

At least with the latest proposal the printing side is completely unaffected. The idea now is to pre-package a va_list-like buffer that can be consumed later.

I was looking at dictionary logging at one point too, and my (somewhat vague) plan was something more like: 1. Put the format string in a separate non-loadable section and give it a unique value at runtime via linker magic 2. Serialize the arguments and the ID in a recoverable way at the point of logging 3. Hand that stuff to an offline python script to figure out 4. Profit

This is almost the case now. The va_list alike chunk only contains pointers to strings that are known to live in read-only memory, or packed into the chunk otherwise. So one could retrieve those chunks and perform the printing offline while omitting the actual printing code from the device entirely.

[jukkar]

size_t len;
CBPRINTF_STATIC_PACKAGE(NULL, &len, "test %d", n);
uint8_t buf[len]; /* len is determined at compile time */
CBPRINTF_STATIC_PACKAGE(buf, &len, "test %d", n);

I do not know enough of this but isn't compiler generating VLA in this case?

[nordic-krch]

@jukkar added following code to hello world:

  size_t len;
  CBPRINTF_STATIC_PACKAGE(NULL, &len, "test %d\n", 1);
  uint8_t buf[len];
  CBPRINTF_STATIC_PACKAGE(buf, &len, "test %d\n", 1);
  cbpprintf(out, NULL, buf);

Got following assemby on arm cortex-m4, compiler is able to calculate size of the package:

LDR         R3, =0x0000405F           /* string address*/
STR         R3, [SP, #8]              /* store it at offset 8 */
MOVS        R3, #1                    /* set argument */
STR         R3, [SP, #12]             /* store it at offset 12 */
MOVS        R1, #0                    /* second argument of cbpprintf is in R1, set to null */
MOVS        R3, #12                   /* size of the package: 12 bytes*/
ADD         R2, SP, #4                /* third argument of cbpprintf is in R2, set to package address SP+4*/
LDR         R0, =out                  /* first argument of cbpprintf */
STRH.W      R3, [SP, #4]              /* Store package length in the first field  uint16_t*/
STRH.W      R1, [SP, #6]              /* set second half to 0 */
BL          cbpprintf                 /* call formatting of the package */

[npitre]

Didn't know about that _Generic thing. That's nice! Looks good overall.

You removed static qualifiers in the unit test file.
I'm kind of guessing why you did that, but mentioning the reason explicitly
in the commit log would be best.

I stopped using VA_STACK_ALIGN_LONGLONG, VA_STACK_ALIGN_DOUBLE, etc.
so to use only VA_STACK_ALIGN(type) directly as nothing needs special
exception for those types. Would be a good idea if you did the same to
avoid possible confusion... and reduce the number of code lines.

Finally, the argument length is now final_length / sizeof(int) and
it is stored in the first uint8_t. I'm not sure if your code matches.
Or I didn't recognize that part.

[nordic-krch]

@npitre

You removed static qualifiers in the unit test file.

Extracted into separate commit.

I stopped using VA_STACK_ALIGN_LONGLONG, VA_STACK_ALIGN_DOUBLE

Removed.

Finally, the argument length is now final_length / sizeof(int) and
it is stored in the first uint8_t.

Code updated.

[nordic-krch]

@pabigot could you take a look?

[pabigot]

Requested changes are elimination of __C_GENERIC and not removing static from internal functions. I don't like reference macro parameters, but won't block on those.

[pabigot]

Do we have existing uses of reference parameters in macros? It's not a natural idiom in C.

[nordic-krch]

that was initially mimicking function call which was taking length by reference. I will remove it.

[nordic-krch]

given #32863 (comment) i'm not sure that i understand now. So since _len is input and output should it be passed by pointer?

[pabigot]

I'm saying that people generally won't expect macro parameters to be both input and output, because you can't do that in functions. The way the behavior is accomplished in functions is to accept a pointer. To use a different idiom for in+out parameters in macros just because they're implemented as textual substitution creates an opportunity for confusion.

This is my opinion, I'm not blocking on it.

[nordic-krch]

updated to have inlen and outlen parameters.

[pabigot]

It might be better to integrate this into the introduction of the API.

Also __C_GENERIC is not a legal identifier for anybody but the toolchain to use. Even if somebody wants to say "but we are the system so we can do whatever we want" it'll confuse people. Z_C_GENERIC maybe.

[nordic-krch]

It might be better to integrate this into the introduction of the API.

do you mean squashing commits?

[pabigot]

yes

[pabigot]

I'm really not a fan of reference parameters in macros.

[nordic-krch]

which one? rc is passed by value

[pabigot]

No, it's not. The reference to what's declared as int rc is passed to TEST_PRF2 as rc which assigned directly to the argument. So it's essentially what in C++ would be int& rc, a reference parameter.

For parity with functions it should be assigned to as *rc = prf(...) in TEST_PRF2, and passed as &rc.

[pabigot]

I really don't like this commit either. We should not have tests/examples that promote contamination of the global namespace with functions that are never referenced outside the translation unit.

If a function's commented out permanently, it should be removed. If it's done during development, ignore the warnings. If it's conditional on build options leave it in the list, and skip on entry when the triggering feature is disabled.

[nordic-krch]

During development commenting a test when running unit test with twister was resulting in errors (it was treat warnings as errors). Do you have suggestion how to handle it there?

[pabigot]

If twister doesn't have an option to override the warnings-are-errors behavior, it should be fixed.

  -W, --disable-warnings-as-errors
                        Treat warning conditions as errors

suggests it does, though the help is rather confusing.

[nordic-krch]

@pabigot comments addressed, except #32863 (comment) which needs clarification.

[nordic-krch]

I also added ac02c98 as a consequence of #33208 discussion. Added assert and recommended to use same alignment as for static packaging.

[pabigot]

Changes have been made. I'm skeptical of the alignment stuff wonder what happens when people intersperse char arguments with long double arguments, but that isn't new so shouldn't affect this PR.

Hopefully the log rework that motivates all this will make it through review.

[npitre]

On Wed, 10 Mar 2021, Peter A. Bigot wrote: I'm skeptical of the alignment stuff wonder what happens when people intersperse char arguments with long double arguments, but that isn't new so shouldn't affect this PR.

It's taken care of. Extra padding is inserted when needed. Of course this is all architecture dependent so this is explicitly tested on all of them.