lib: cbprintf: don't use double va_arg if no fp support

[jharris-intel]

Even though this portion of the code is unreachable, the
compiler cannot figure that out, and so produces floating-point
operations even if CBPRINTF_FP_SUPPORT is off. Give the compiler
a hand by explicitly disabling this code if CBPRINTF_FP_SUPPORT
is off.

Signed-off-by: James Harris [email protected]

[jharris-intel]

@pabigot - re: "Please add me as a reviewer when that's submitted if github doesn't do so." at #32192 (comment)

FYI.

[pabigot]

This isn't going to work, as it will break subsequent arguments when floating point arguments are passed but not converted.

This can be demonstrated by modifying the hello_world sample to be:

printk("Hello %f %x World! %s\n", 2.5, 0x12345678, CONFIG_BOARD);

With the current implementation and FP disabled we get:

*** Booting Zephyr OS build zephyr-v2.5.0-187-g4b5a2b989eee  ***
Hello %f 12345678 World! nrf52840dk_nrf52840

With the implementation in this PR we get:

*** Booting Zephyr OS build zephyr-v2.5.0-187-g4b5a2b989eee  ***
Hello %f e000ed00 World! 

[pabigot]

I don't believe this is correct

While it's true the potential float arguments won't be used if CBPRINTF_FP_SUPPORT isn't enabled, they must be consumed from the va_list otherwise subsequent formatting operations would be performed using values that aren't what they correspond to.

If the mere reference to floating point types without evaluating them is going to cause problems, we need some other way of popping an object of the corresponding double or long double size off the va_list.

Alternatively, we could have a condition here on CONFIG_FLOAT_UNSUPPORTED=y if/when that gets added, justified by an expectation that on such a platform there's no way to pass a float, and the prayer that nobody will nonetheless include a floating point specifier in the format string.

[jharris-intel]

This isn't going to work, as it will break subsequent arguments when floating point arguments are passed but not converted.

This can be demonstrated by modifying the hello_world sample to be:

printk("Hello %f %x World! %s\n", 2.5, 0x12345678, CONFIG_BOARD);

With the current implementation and FP disabled we get:

*** Booting Zephyr OS build zephyr-v2.5.0-187-g4b5a2b989eee  ***
Hello %f 12345678 World! nrf52840dk_nrf52840

With the implementation in this PR we get:

*** Booting Zephyr OS build zephyr-v2.5.0-187-g4b5a2b989eee  ***
Hello %f e000ed00 World! 

Oof. I didn't realize that the code attempted to parse subsequent arguments after an unsupported argument. Probably should have, sorry about that.

Would bailing out and not parsing any arguments after the floating-point argument in this case if CONFIG_FLOAT_UNSUPPORTED=y be an acceptable compromise here? (Requires delaying or opening another PR, because as you mention CONFIG_FLOAT_UNSUPPORTED doesn't currently exist.)

---

we need some other way of popping an object of the corresponding double or long double size off the va_list.

Unfortunately, there's no other way of doing so that I know of. In particular, simply popping another type of the same size off of the list does not work in general, as e.g. AArch32's va_args calling convention has a separate stack for floating-point types (...ish. It's a mite complex.)

See e.g. https://godbolt.org/z/76v4sh.

[pabigot]

Would bailing out and not parsing any arguments after the floating-point argument in this case if CONFIG_FLOAT_UNSUPPORTED=y be an acceptable compromise here?

It would depend on what we decided CONFIG_FLOAT_UNSUPPORTED meant. A natural interpretation is "There will be no [edited] support for performing floating point calculations", which doesn't necessarily outlaw the ability to format floating point values that might be received without being used in calculations (e.g. a device that receives sensor data and displays it).

The toolchain you're using is the first one identified that seems to get its knickers twisted at the mere appearance of float types even in situations where the value is not used in floating point operations. It's a pretty extreme position to take. So I'd tentatively prefer to say that the condition of "if !FP_SPECIFIER fail to convert anything more" should be specific to toolchains that absolutely require it. Which might have to be signaled by CONFIG_FLOAT_FORBIDDEN, which could justifiably have the proposed behavior.

simply popping another type of the same size off of the list does not work in general

OK, then, that won't fly.

[jharris-intel]

The toolchain you're using is the first one identified that seems to get its knickers twisted at the mere appearance of float types even in situations where the value is not used in floating point operations

Yeah. It's a bit of a niche I am well aware.

(To be slightly pedantic: it's "any target without a defined soft-float or hard-float ABI". Most other ABIs have at least one of the two.)

CONFIG_FLOAT_UNSUPPORTED
CONFIG_FLOAT_FORBIDDEN

What would be allowed by the former but forbidden by the latter? I don't know of any configuration that allows va_arg(float that doesn't allow float_val + 1.0 or vice versa.

Could you give e.g. a godbolt link of the distinction?

[pabigot]

CONFIG_FLOAT_UNSUPPORTED
CONFIG_FLOAT_FORBIDDEN

What would be allowed by the former but forbidden by the latter? I don't know of any configuration that allows va_arg(float that doesn't allow float_val + 1.0 or vice versa.

Consider:

printk("Temp=%.2g Cel at %02d:%02d\n", -11.8, 17, 7);

We have a general history of wanting to say "I'm not going to use floating point" and having that mean select flags so that newlib doesn't include support, and cbprintf doesn't add support, but not intentionally breaking any hidden FP that might be supported by the toolchain. That'd be UNSUPPORTED: output should be"Temp=%.2g at 17:07\n".

FORBIDDEN would be special for anything where the mere appearance of using floating point means things that would otherwise work are allowed to do something unexpected instead. Output might be "", or "Temp=%02g", or "Temp=%02g at %02d:%02d\n", depending on how we decide to handle that.

[jharris-intel]

Sorry, I am failing to understand.

That behavior is toggled by CONFIG_CBPRINTF_FP_SUPPORT, no?

Or, to put it another way, what's the difference between CONFIG_FLOAT_SOFT=y+CONFIG_CBPRINTF_FP_SUPPORT=n and CONFIG_FLOAT_UNSUPPORTED=y+CONFIG_CBPRINTF_FP_SUPPORT=n in your example?

[pabigot]

CONFIG_FLOAT_* can be used to default CONFIG_CBPRINTF_FP_SUPPORT and other flags (newlib) to reduce the amount of customization applications need to do.

[jharris-intel]

Marked as draft because of the very good point @pabigot brought up above.

[jharris-intel]

I'm going to open a separate PR for CONFIG_FLOAT_FORBIDDEN.