libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled

[xiaoxiang781216]

Summary

but just format the low 32bits if CONFIG_LIBC_LONG_LONG isn't enabled to
avoid to expand the code space to much.
Note: the size will increase 192 bytes on stm32_tiny:nsh.
Before the change:

   text    data     bss     dec     hex filename
  41444     184    1656   43284    a914 nuttx

After the change:

   text    data     bss     dec     hex filename
  41636     184    1656   43476    a9d4 nuttx

Impact

Fix the complain in #6606

Testing

Pass CI

[pkarashchenko]

The PR seems to be addressing initial concern on size increase and keeping things consistent.
LGTM!

[acassis]

@xiaoxiang781216 @pkarashchenko @ALTracer I think the code should use CONFIG_LIBC_LONG_LONG instead of CONFIG_HAVE_LONG_LONG and we need to fix CONFIG_LIBC_LONG_LONG definition on Kconfig to be enabled by default if compiler has support to long long (CONFIG_HAVE_LONG_LONG) and DEFAULT_SMALL is not used. So CONFIG_HAVE_LONG_LONG shouldn't be an entry on Kconfig, but only defined at include/nuttx/compiler.h

We need to fix the Kconfig to:

config LIBC_LONG_LONG
        bool "Enable long long support in printf"
        default y if HAVE_LONG_LONG && !DEFAULT_SMALL

It will avoid the issues pointed by @pkarashchenko

[pkarashchenko]

The current PR reduces code size by disabling long long support for printf (I will double check scanf) and still keeps support of %ll and %ull by mapping those to %l and %ul. I think that it addresses the size increase concern raised in the previous PR.
@acassis I'm not sure if the Kconfig example that you mentioned will work since HAVE_LONG_LONG is not a Kconfig option (at least for now)

[acassis]

The current PR reduces code size by disabling long long support for printf (I will double check scanf) and still keeps support of %ll and %ull by mapping those to %l and %ul. I think that it addresses the size increase concern raised in the previous PR. @acassis I'm not sure if the Kconfig example that you mentioned will work since HAVE_LONG_LONG is not a Kconfig option (at least for now)

Good point! My fault, in fact CONFIG_HAVE_LONG_LONG is defined by the header file. So, maybe we can get rid of CONFIG_LIBC_HAVE_LONG_LONG and only use CONFIG_HAVE_LONG_LONG, because if the system doesn't have it, it will fallback to LONG.

[pkarashchenko]

The original complaint was that there is a need to perform computation in long long, but disable long long printing in LIBC.

[acassis]

The original complaint was that there is a need to perform computation in long long, but disable long long printing in LIBC.

Yes, and since now it can print correctly even when LIBC_LONG_LONG is not enabled and will increase only 192 bytes, this seems a nice solution! @ALTracer do you agree? I know you spent time implementing other fixes, but this solution is easier and solves the original "df -h" issue.

[xiaoxiang781216]

The current PR reduces code size by disabling long long support for printf (I will double check scanf) and still keeps support of %ll and %ull by mapping those to %l and %ul. I think that it addresses the size increase concern raised in the previous PR. @acassis I'm not sure if the Kconfig example that you mentioned will work since HAVE_LONG_LONG is not a Kconfig option (at least for now)

Good point! My fault, in fact CONFIG_HAVE_LONG_LONG is defined by the header file. So, maybe we can get rid of CONFIG_LIBC_HAVE_LONG_LONG and only use CONFIG_HAVE_LONG_LONG, because if the system doesn't have it, it will fallback to LONG.

Once we move CONFIG_HAVE_LONG_LONG to Kconfig, we can apply @acassis 's suggestion to simplify the default setting. Look like @ALTracer will provide a patch for this.

[ALTracer]

The original complaint was that there is a need to perform computation in long long, but disable long long printing in LIBC.

Yes, and since now it can print correctly even when LIBC_LONG_LONG is not enabled and will increase only 192 bytes, this seems a nice solution! @ALTracer do you agree? I know you spent time implementing other fixes, but this solution is easier and solves the original "df -h" issue.

This PR is better than #6606, but I still have questions.

@xiaoxiang781216 Does printf now parse ull as ul? What if ull argument is > ULONG_MAX? Who should deal with overflows -- calling site or printf (with debug warnings before casting / consuming va_arg)?

I'm fine with code size like that. Apparently, it's coming only from libc internals now.

[ALTracer]

Once we move CONFIG_HAVE_LONG_LONG to Kconfig, we can apply @acassis 's suggestion to simplify the default setting. Look like @ALTracer will provide a patch for this.

In light of your recent refactoring of most toolchain flags in Kconfig tree and #6123 bringing LTO support, -- yes, I think I'd like to add Kconfig options for HAVE_LONG_LONG and friends (HAVE_FLOAT, HAVE_DOUBLE, HAVE_LONG_DOUBLE) somewhere under Optimizations (where FRAME_POINTERS are) or under System Type (where LTO is).
Of course it's default y and depends on TOOLCHAIN_GCC/CLANG/BUILDROOT or whatever it is now (I'm still sitting on release/10.3 branch with my project). Maybe I'll even hide it with CONFIG_EXPERIMENTAL.

While it's silly to disable FLOAT, for example, I think it's for user to decide to drop DOUBLE. Like C++ has a principle "You only pay for what you use".

[xiaoxiang781216]

The original complaint was that there is a need to perform computation in long long, but disable long long printing in LIBC.

Yes, and since now it can print correctly even when LIBC_LONG_LONG is not enabled and will increase only 192 bytes, this seems a nice solution! @ALTracer do you agree? I know you spent time implementing other fixes, but this solution is easier and solves the original "df -h" issue.

This PR is better than #6606, but I still have questions.

@xiaoxiang781216 Does printf now parse ull as ul?

Yes, it will handle both.

What if ull argument is > ULONG_MAX? Who should deal with overflows -- calling site or printf (with debug warnings before casting / consuming va_arg)?

it's simply discard the high 32bit, which mean the output is correct if value is less/equal than ULONG_MAX, but wrong if great than ULONG_MAX.

I'm fine with code size like that. Apparently, it's coming only from libc internals now.

[hartmannathan]

Once we move CONFIG_HAVE_LONG_LONG to Kconfig, we can apply @acassis 's suggestion to simplify the default setting. Look like @ALTracer will provide a patch for this.

In light of your recent refactoring of most toolchain flags in Kconfig tree and #6123 bringing LTO support, -- yes, I think I'd like to add Kconfig options for HAVE_LONG_LONG and friends (HAVE_FLOAT, HAVE_DOUBLE, HAVE_LONG_DOUBLE) somewhere under Optimizations (where FRAME_POINTERS are) or under System Type (where LTO is). Of course it's default y and depends on TOOLCHAIN_GCC/CLANG/BUILDROOT or whatever it is now (I'm still sitting on release/10.3 branch with my project). Maybe I'll even hide it with CONFIG_EXPERIMENTAL.

While it's silly to disable FLOAT, for example, I think it's for user to decide to drop DOUBLE. Like C++ has a principle "You only pay for what you use".

It's not always silly to disable FLOAT. Depends on what you need. Various embedded libc support a choice of printf() that supports "%f" vs a printf() that doesn't support it, to save code size (by choosing which version of the library to link). That's up to each application to decide what it needs. Personally I am in favor of allowing NuttX to be reduced to very minimal sizes for certain things, and I also like that you can turn on all the bells and whistles for bigger embedded systems.

[pkarashchenko]

Yes, the GCC printf nano version has an option to enable FLOAT printing (both float and double) to reduce code size.
The case with long long here is a bit more complicated. This PR still try to print "at least" instead of ignoring %ll/%ull at all, so it is not "pure" disable for long long (like disabling FLOAT print in GCC libc nano). That is a bit confusing as with "disable" of CONFIG_LIBC_HAVE_LONG_LONG the %ll/%ull should be "disabled" and not "truncated".

[hartmannathan]

Yes, the GCC printf nano version has an option to enable FLOAT printing (both float and double) to reduce code size.
The case with long long here is a bit more complicated. This PR still try to print "at least" instead of ignoring %ll/%ull at all, so it is not "pure" disable for long long (like disabling FLOAT print in GCC libc nano). That is a bit confusing as with "disable" of CONFIG_LIBC_HAVE_LONG_LONG the %ll/%ull should be "disabled" and not "truncated".

Won't this create confusion and weird bugs? E.g., if a value > INT32_MAX is passed in, it will be truncated, printing wrong results?

[ALTracer]

While it's silly to disable FLOAT, for example, I think it's for user to decide to drop DOUBLE. Like C++ has a principle "You only pay for what you use".

It's not always silly to disable FLOAT. Depends on what you need. Various embedded libc support a choice of printf() that supports "%f" vs a printf() that doesn't support it, to save code size (by choosing which version of the library to link). That's up to each application to decide what it needs. Personally I am in favor of allowing NuttX to be reduced to very minimal sizes for certain things, and I also like that you can turn on all the bells and whistles for bigger embedded systems.

@hartmannathan I understand what you're referring to. I personally use newlib-nano without -u_printf_float because otherwise it adds a whopping 16k .text of float/double parsing [printing] code and some stack requirements. I didn't mean to stop developers from using the float and double c datatypes and hardfp VFP unit if they have it. Softfloat, on the other hand, can become expensive.

@pkarashchenko I second your opinion in that ull behaviour is funkier. I personally would like to stare at the ull argument and issue an overflow warning before casting the va_arg down to long and printing the lower half anyways. Otherwise we might introduce undefined behaviour in signed overflow.

From another point of view, some IDEs for embedded often highlight printf("%f") attempts when the project specifically has newlib-nano with no printf-float logic. The runtime silently skips all %f arguments, IIRC. Maybe it's better to skip parsing ll/ull altogether because "we don't support it"?
Or maybe we could refactor all call-sites trying to print ull via our libc instead? This issue (and friends) arises in part because some intermediate internal calculations are carried out in [u]int64_t types, but then are (or are not) casted down to [u]int32_t in printf calls.

[mlyszczek]

How about?

#ifndef CONFIG_LIBC_LONG_LONG
DEBUGASSERT(strcmp(fmt, "%ll"));
#endif

This has 0 cost in production, and you usually want to run your code with DEBUGASSERT() enabled before releasing, so programmer will be notified about trying to print ll when ll is not supported. Then, if programmer wants to ignore this, responsibility of broken code lies on him - and not on nuttx. This of course forces solution where nuttx simply don't print ll without CONFIG_LIBC_LONG_LONG.

If you want to get creative, you could also add trivial strings nuttx | grep -E "%u?ll" to build process if CONFIG_LIBC_LONG_LONG is not defied and warn user about the situation.

With these solutions after error/warning programmer can either enable CONFIG_LIBC_LONG_LONG or adapt code to use only int.

This wouldn't be acceptable in Linux world, but in small embedded with rtos? I think it's acceptable - and maybe even desired as it saves space.

[xiaoxiang781216]

Yes, the GCC printf nano version has an option to enable FLOAT printing (both float and double) to reduce code size.
The case with long long here is a bit more complicated. This PR still try to print "at least" instead of ignoring %ll/%ull at all, so it is not "pure" disable for long long (like disabling FLOAT print in GCC libc nano). That is a bit confusing as with "disable" of CONFIG_LIBC_HAVE_LONG_LONG the %ll/%ull should be "disabled" and not "truncated".

Won't this create confusion and weird bugs? E.g., if a value > INT32_MAX is passed in, it will be truncated, printing wrong results?

Yes, if you decide to disable CONFIG_LIBC_HAVE_LONG_LONG. It is impossible to get the small code size and the more functionality at the same time, but we give you the choice.

[xiaoxiang781216]

How about?

#ifndef CONFIG_LIBC_LONG_LONG
DEBUGASSERT(strcmp(fmt, "%ll"));
#endif

This has 0 cost in production, and you usually want to run your code with DEBUGASSERT() enabled before releasing, so programmer will be notified about trying to print ll when ll is not supported. Then, if programmer wants to ignore this, responsibility of broken code lies on him - and not on nuttx. This of course forces solution where nuttx simply don't print ll without CONFIG_LIBC_LONG_LONG.

It isn't good since there are many places which use uint64_t when possible, but the result is seldom larger than ULONG_MAX, or the user may prefer to save the coe space and then accept the wrong printf output(e.g. log or procfs).

If you want to get creative, you could also add trivial strings nuttx | grep -E "%u?ll" to build process if CONFIG_LIBC_LONG_LONG is not defied and warn user about the situation.

With these solutions after error/warning programmer can either enable CONFIG_LIBC_LONG_LONG or adapt code to use only int.

This wouldn't be acceptable in Linux world, but in small embedded with rtos? I think it's acceptable - and maybe even desired as it saves space.

But, I add the overflow check before down cast:
https://github.com/apache/incubator-nuttx/pull/6613/files#diff-befc8e04ddb3722f024f65ffd7648aac1cb24846d2b80fff6328e3cb5862220fR1042-R1044
https://github.com/apache/incubator-nuttx/pull/6613/files#diff-befc8e04ddb3722f024f65ffd7648aac1cb24846d2b80fff6328e3cb5862220fR1217-R1219
which could notice the user some bad thing happen in debugging mode.

[hartmannathan]

How about?

#ifndef CONFIG_LIBC_LONG_LONG
DEBUGASSERT(strcmp(fmt, "%ll"));
#endif

This has 0 cost in production, and you usually want to run your code with DEBUGASSERT() enabled before releasing, so programmer will be notified about trying to print ll when ll is not supported. Then, if programmer wants to ignore this, responsibility of broken code lies on him - and not on nuttx. This of course forces solution where nuttx simply don't print ll without CONFIG_LIBC_LONG_LONG.

If you want to get creative, you could also add trivial strings nuttx | grep -E "%u?ll" to build process if CONFIG_LIBC_LONG_LONG is not defied and warn user about the situation.

With these solutions after error/warning programmer can either enable CONFIG_LIBC_LONG_LONG or adapt code to use only int.

This wouldn't be acceptable in Linux world, but in small embedded with rtos? I think it's acceptable - and maybe even desired as it saves space.

@mlyszczek I like this idea. This (or something similar) could really save developers a lot of frustration. If it works for %llu then the same idea could work for %f when printf() is compiled without support for printing floats. Try to print a float and you'll get an error. This feature could be made optional by toggling a Kconfig under Debug Options.

Another alternative, if CONFIG_LIBC_LONG_LONG is not defined, is to print #error when an unsupported format specifier is used. I recall at least one libc that does that for %f when not supported.

In my opinion, it is better to print something that obviously indicates an error than to risk printing incorrect information (which might be consumed by other programs leading to very hard-to-find bugs) by truncating a uint64_t and printing just the lower 32 bits.

I understand the point @xiaoxiang781216 is making that usually a 64-bit variable would contain a 32-bit number. I would be open to compromising and printing the lower 32 bits IF the upper 32 bits are 0 and printing #error if any upper bits are 1.

The point is: Either print CORRECT information or print an obvious error. But don't print incorrect information under any circumstances.

[xiaoxiang781216]

@hartmannathan the patch already add the debug assert in case the value > ULONG_MAX.

[mlyszczek]

@hartmannathan the patch already add the debug assert in case the value > ULONG_MAX.

It does, but in unpredictable way. Assert will only trigger if in debug runtime, you try to print something bigger than uint32_t, and this may not trigger in debug, but will malform data on production.

Second thing is, "x" is signed, and comparision is to unsigned. if (-1ll < ULONG_MAX) will be false negative thus triggering assert, in fact, half of the negative numbers will trigger assert here (assuming sizeof(long) != sizeof(long long), when they are equal - only -1 will trigger false assert).

I have a little anxiety with these runtime checks. These checks may never trigger in a debug build, but on production variable might suddenly (and we all know it eventually will:)) get bigger than long, and someday this will ruin somebody's day. Note, that these checks are not only for mere printf(), but also for sprintf() which in fact may be used for serious business (but so can fprintf(), ie. to pass big number to different CPU via serial link).

Safeset approach would be "you want to print %ll? enable support for it or patch your code to include dangerous stuff yourself". If there are places where long long is used and (s)printfed, but we don't care if they overflow or not, these values should be printed as %l and explicitly casted down to (long). Anything else seems like a time bomb.

@hartmannathan just out of curiosity, how do you want to print #error when "%ll" was used? Unless I missunderstood you and this is not compiler #error, I don't think preprocesor can detect "%ll" in string?

[xiaoxiang781216]

@hartmannathan the patch already add the debug assert in case the value > ULONG_MAX.

It does, but in unpredictable way. Assert will only trigger if in debug runtime, you try to print something bigger than uint32_t, and this may not trigger in debug, but will malform data on production.

Before you ship the product, you must do the full test. If you don't enable CONFIG_LIBC_LONG_LONG, but print the number larger than 32bits, nobody can help you, either assert or panic can't give your the right answer.

Second thing is, "x" is signed, and comparision is to unsigned. if (-1ll < ULONG_MAX) will be false negative thus triggering assert, in fact, half of the negative numbers will trigger assert here (assuming sizeof(long) != sizeof(long long), when they are equal - only -1 will trigger false assert).

It's easy to fix, please review again.

I have a little anxiety with these runtime checks. These checks may never trigger in a debug build, but on production variable might suddenly (and we all know it eventually will:)) get bigger than long, and someday this will ruin somebody's day. Note, that these checks are not only for mere printf(), but also for sprintf() which in fact may be used for serious business (but so can fprintf(), ie. to pass big number to different CPU via serial link).

Debug check just help the people to pinpoint the issue quickly. It's wrong to printf the number > 32bits without CONFIG_LIBC_LONG_LONG == y. The only fix is to modify your config to enable CONFIG_LIBC_LONG_LONG.

Safeset approach would be "you want to print %ll? enable support for it or patch your code to include dangerous stuff yourself". If there are places where long long is used and (s)printfed, but we don't care if they overflow or not, these values should be printed as %l and explicitly casted down to (long). Anything else seems like a time bomb.

This patch doesn't forbid you correct the code at the call site. If you really want to do so, please provide the patch to fix them case by case. But please don't add the warning log any more, because this patch already do the assertion in the wrong case.

@hartmannathan just out of curiosity, how do you want to print #error when "%ll" was used? Unless I missunderstood you and this is not compiler #error, I don't think preprocesor can detect "%ll" in string?

[mlyszczek]

Yea, there won't be a "-1" bug now, but this will trigger an assert for unsigned ll bigger than ULONG_MAX/2. I guess you could do:

DEBUGASSERT((LONG_MIN <= x && x <= LONG_MAX) || (0 < x && x <= ULONG_MAX));

To handle both signed and unsigned cases.

[xiaoxiang781216]

Yea, there won't be a "-1" bug now, but this will trigger an assert for unsigned ll bigger than ULONG_MAX/2. I guess you could do:

DEBUGASSERT((LONG_MIN <= x && x <= LONG_MAX) || (0 < x && x <= ULONG_MAX));

To handle both signed and unsigned cases.

Done.

[ALTracer]

Yea, there won't be a "-1" bug now, but this will trigger an assert for unsigned ll bigger than ULONG_MAX/2. I guess you could do:

DEBUGASSERT((LONG_MIN <= x && x <= LONG_MAX) || (0 < x && x <= ULONG_MAX));

To handle both signed and unsigned cases.

Now I don't understand it. Can you explain how does this assert work?
Do integer comparisons depend on signedness in C? Then it should be 0UL..? What is the type of x and does it matter (type promotion)?

I naively think atm that this assert checks whether 64-bit x is a valid (sign-extended representation of a) 32-bit integer from LONG_MIN to ULONG_MAX inclusively.

[mlyszczek]

Yea, there won't be a "-1" bug now, but this will trigger an assert for unsigned ll bigger than ULONG_MAX/2. I guess you could do:
DEBUGASSERT((LONG_MIN <= x && x <= LONG_MAX) || (0 < x && x <= ULONG_MAX));
To handle both signed and unsigned cases.

Now I don't understand it. Can you explain how does this assert work? Do integer comparisons depend on signedness in C? Then it should be 0UL..? What is the type of x and does it matter (type promotion)?

You can't compare signed and unsigned. If you try to compare signed and unsigned, signed value will be converted to unsigned. And so, considering 8bits vars, -1 < UCHAR_MAX will return false, when you would expect it to return true. Reason for this is, that netagive numbers are represented as 2 complement, so -1 in bit representation will be 11111111 (-2 will be 11111110 and so on) which is equal to UCHAR_MAX.

(LONG_MIN <= x && x <= LONG_MAX)
This part only checks if x is within signed values, so around +-2million. Unsigned values can be bigger and can be up to around 4million. If you'd pass 4million value, assert would trigger as 4million is bigger than 2million. That's where the second part comes in.

(0 < x && x <= ULONG_MAX)
It simply checks, if number is bigger than 0, then check if it's lower than 4million. So for 4million value, first part will return false but second will return true so whole assert will be true and it won't trigger.

0 in that case will be promoted to unsigned long.

I naively think atm that this assert checks whether 64-bit x is a valid (sign-extended representation of a) 32-bit integer from LONG_MIN to ULONG_MAX inclusively.

[ALTracer]

You can't compare signed and unsigned. If you try to compare signed and unsigned, signed value will be converted to unsigned. And so, considering 8bits vars, -1 < UCHAR_MAX will return false, when you would expect it to return true. Reason for this is, that netagive numbers are represented as 2 complement, so -1 in bit representation will be 11111111 (-2 will be 11111110 and so on) which is equal to UCHAR_MAX.

Thanks for the refresher. The logic is always nice and understandable with small values and intervals.

(LONG_MIN <= x && x <= LONG_MAX) This part only checks if x is within signed values, so around +-2million. Unsigned values can be bigger and can be up to around 4million. If you'd pass 4million value, assert would trigger as 4million is bigger than 2million. That's where the second part comes in.

That is usually right. However, vsprintf flips the sign before passing the x to
__ultoa_invert(unsigned long long val, char str, int base) (and before the first assert)
https://github.com/apache/incubator-nuttx/blob/8f465c43cee41c99d857c8cf838c0d86de4ff186/libs/libc/stdio/lib_libvsprintf.c#L1032

          if (x < 0)
            {
              x = -x;
              flags |= FL_NEGATIVE;
            }

(0 < x && x <= ULONG_MAX) It simply checks, if number is bigger than 0, then check if it's lower than 4million. So for 4million value, first part will return false but second will return true so whole assert will be true and it won't trigger.

There are two asserts against differently typed x: first is long long x in %d/%i branch, second is unsigned long long x in %u branch. Why should we check a %u argument against int32_t limits anyways?

0 in that case will be promoted to unsigned long.

No, because x is long long there (with sign already flipped to positive) and 0 is int by default.
Yes, because x is unsigned long long there and 0 will be promoted to its type.

[mlyszczek]

Yeah, of course long long, I meant that 0 will be promoted to unsigned, didn't pay attention to type:) you're right.

I admit I didn't dive into the code that much and did focus only at that single line. If we are sure we are dealing with signed type then yes, you are right, second part is not necessary in that case.

[xiaoxiang781216]

Ok, I remove the second check from DEBUGASSERT.

[xiaoxiang781216]

@ALTracer @mlyszczek do you have more concern for these PR(#6613, #6607 and apache/nuttx-apps#1221)? if no, @pkarashchenko you can merge these patch now.

[ALTracer]

@xiaoxiang781216 No more questions from me, LGTM on all three PRs. Time will tell, let's merge to master and let bleeding edge users test.
Next NuttX release will be really awesome after all the optimizations since 10.3 .

[mlyszczek]

Yep, it LGTM too, just like @ALTracer said, time will unravel how this plays out and if we get another bug report;)

[xiaoxiang781216]

@pkarashchenko could you merge all related PR?

[pkarashchenko]

@pkarashchenko could you merge all related PR?

I will review again and merge today.

[hartmannathan]

LGTM