You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/07/14 16:43:09 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
xiaoxiang781216 opened a new pull request, #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613
## 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
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ALTracer commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185258059
> 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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1184871825
The original complaint was that there is a need to perform computation in long long, but disable long long printing in LIBC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] mlyszczek commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186433174
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] mlyszczek commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186546988
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1189902641
> @pkarashchenko could you merge all related PR?
I will review again and merge today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ALTracer commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185737281
> > 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._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ALTracer commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186590308
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] mlyszczek commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1188738356
Yep, it LGTM too, just like @ALTracer said, time will unravel how this plays out and if we get another bug report;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1184856065
@xiaoxiang781216 @pkarashchenko @ALTracer I thing 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). 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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ALTracer commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185238395
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1189901877
@pkarashchenko could you merge all related PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1188686001
@ALTracer @mlyszczek do you have more concern for these PR(https://github.com/apache/incubator-nuttx/pull/6613, https://github.com/apache/incubator-nuttx/pull/6607 and https://github.com/apache/incubator-nuttx-apps/pull/1221)? if no, @pkarashchenko you can merge these patch now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185295923
> > > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186553363
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1184868111
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1184878896
> 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186195406
> 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 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] mlyszczek commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186614206
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1184862602
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186410586
@hartmannathan the patch already add the debug assert in case the value > ULONG_MAX.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185102582
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185573352
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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] mlyszczek commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186143465
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ALTracer commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186605916
> 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
```c
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186193698
> > 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 never possible to get the small code size and the more functionality at the same time, but we give you the choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] mlyszczek commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186594715
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186750441
Ok, I remove the second check from DEBUGASSERT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ALTracer commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1188691974
@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 ._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186377358
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1186446270
> > @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.
> 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).
>
> 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 do the check at the call site. If you really want to do so, please provide the patch to fix them case by 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185593547
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6613: libc/stdio: Support "long long" type if CONFIG_HAVE_LONG_LONG is enabled
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6613:
URL: https://github.com/apache/incubator-nuttx/pull/6613#issuecomment-1185565868
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org