eyalroz / printf Goto Github PK

I don't like magic numbers. And while it's more difficult to get rid of them in lower-level code, and in C rather than C++ - we should still make an effort.

Related to #46 .

At the moment, we pass an ssize_t to "%zi" when testing. But... there is no ssize_t on Windows, per se. Let's try to scrape one from somewhere.

We need to have our tests/CMakeFiles.txt allow for CUDA targets and their build. And probably make that optional so that this stuff doesn't get built by default for non-CUDA-users.

Hello! Thank you, @eyalroz, for continuing the project.

I prefer the fork, since it supports CMake. Likewise, I use it with FetchContent:

function(ProvidePrintfLibrary)

    include(FetchContent)

    set(BUILD_STATIC_LIBRARY ON)
    set(SUPPORT_EXPONENTIAL_SPECIFIERS OFF)
    set(SUPPORT_LONG_LONG OFF)
    set(ALIAS_STANDARD_FUNCTION_NAMES OFF)
    set(DEFAULT_FLOAT_PRECISION "3")
    set(MAX_INTEGRAL_DIGITS_FOR_DECIMAL "3")

    FetchContent_Declare(printf_library
        GIT_REPOSITORY https://github.com/eyalroz/printf.git
        GIT_TAG v5.0.0
    )
    FetchContent_MakeAvailable(printf_library)

    FetchContent_GetProperties(printf_library SOURCE_DIR printf_source_dir)
    target_include_directories(printf PUBLIC $<BUILD_INTERFACE:${printf_source_dir}>)
    
endfunction()

I would like to address some miscellaneous issues which I had to bypass:

1. I use the function here to set the local variables to not mutate the parent scope. "Normal variables" are used instead of options. There might be a risk that the name of BUILD_STATIC_LIBRARY may clash with other libraries/dependencies - it's quite general. I would prefer to make the name more specific by adding a PRINTF_ prefix to the options, so the variables could be preset by the user also with the option command.
2. I had to propagate BUILD_INTERFACE include directory, because using #include "printf.h" causes a compile error, since no include directories are propagated by the library.
3. Some variables in the config mismatch the names used in the code. One example is PRINTF_INTEGER_BUFFER_SIZE CMake option. In the code, it is PRINT_INTEGER_BUFFER_SIZE. It means that the change to the former doesn't affect the latter.

I will create a PR that addresses the issue.

Looking over the code, I noted that there is apparent support of 32b doubles (via DOUBLE_SIZE_IN_BITS and friends), and avoid making use of (u)int64_t in that case. But: my compiler supports only 32b ints (as well as only 32b doubles). Does that mean I am out of luck if I want to define PRINTF_SUPPORT_DECIMAL_SPECIFIERS
or PRINTF_SUPPORT_EXPONENTIAL_SPECIFIERS? The code makes use of int_fast64_t, which size I can't do. Has this issue been discussed? (I'd mark this as a question but do not know how)

The repository root has a bunch of non-source files in addition to printf.c and printf.h; this is slightly distracting and messy. In larger repositories, one typically places the sources files in (one or more) subdirectories, and none of them in the repository root.

Should we, then, create a src/source subdirectory, and movie printf.h and printf.c into it?

The following build warning occurs with:
PRINTF_SUPPORT_FLOAT_SPECIFIERS ON
PRINTF_SUPPORT_EXPONENTIAL_SPECIFIERS OFF
PRINTF_SUPPORT_LONG_LONG ON

Built with arm-none-eabi-gcc (15:7-2018-q2-6) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]

app/src/printf.c: In function 'sprint_floating_point':
app/src/printf.c:779:10: warning: implicit declaration of function 'sprint_exponential_number'; did you mean 'sprint_decimal_number'? [-Wimplicit-function-declaration]
sprint_exponential_number(out, buffer, idx, maxlen, value, precision, width, flags, buf, len) :
^~~~~~~~~~~~~~~~~~~~~~~~~
sprint_decimal_number

On most machines, printf("%d", INT_MIN) will cause a signed integer overflow, resulting in undefined behavior.

At line 782 the absolute value is computed with (unsigned int)(value > 0 ? value : 0 - value). However when VALUE == INT_MIN on a typical twos-complement machine, then 0 - value is -INT_MIN which cannot be represented as int and so overflow occurs. -value would have the same issue. (For instance, on a typical 32-bit machine int can represent -2147483648 as 0x80000000, but cannot represent 2147483648.)

Compile printf.c with gcc -ftrapv or gcc -fsanitize=undefined and you'll get a runtime error for this example.

There is the same problem for printf("%ld", LONG_MIN) and printf("%lld", LLONG_MIN).

I think it should instead be value > 0 ? (unsigned int)value : -(unsigned int)value. Converting a negative int to unsigned effectively adds UINT_MAX (typically 2**32) and negating subtracts from UINT_MAX, yielding the correct result. Unsigned integer overflow in C is well defined. There will still be a problem on a machine where -INT_MIN doesn't fit in unsigned int but they should be rare. (You could cast up to unsigned long long which is a little better, but you will still have the same problem with -LLONG_MIN.)

By the way, these cases are not exercised in the test suite, which might be a good idea. It's a little tricky to add because we don't know the value of INT_MIN ahead of time, so we don't know what string to expect, but we also can't safely do printf("%d", -2147483648) because we could be on a machine with 16-bit int.

PRINTF_INTEGER_VALUE_TYPE... what was I thinking?

1. It's a macro instead of a typedef.
2. It's an unsigned type calling itself an "integer type" (ok, only slightly confusing).
3. We cast to it in PRINTF_ABS() ... and then apply a minus. The fact that this hasn't failed my implementation is not much than a happy fluke... :-(

At the moment, the library always aliases printf, sprintf, vsprintf etc. This is problematic, because you might want to use it alongside the standard library's <stdio.h> (whether because you're using the standard library as well, or because you're including another header which itself pulls <stdio.h> without you really needing it).

So, we should have a CMake option controlling whether or not this is done.

printf.h tests for PRINTF_ALIAS_STANDARD_FUNCTION_NAMES , but can't see it as it's defined in printf_config.h

Enabling option PRINTF_SUPPORT_EXPONENTIAL_SPECIFIERS but not PRINTF_SUPPORT_FLOAT_SPECIFIERS.
Yields neither..
This may occur if an application really only wants the exponent format.

Let's update the catch2 version to something more recent. We're at catch 2.7.0, and the latest release is 2.13.6. Might not be critical, but - I'm sure they've improved things since 2019.

The motivation for switching from decimal to exponential notation beyond a certain value (and breaking the standard's requirements) is the need to avoid overflowing the buffers. Therefore, the limit/threshold value should be stated in terms of buffer space, or in other words - decimal digits, each of which take up a character in the buffer.

So, let's switch to a number of digits.

With commit 51c90f9 (before v5.0.0), I made support for the %t specifier always enabled; yet - I didn't update README.md to reflect this. Thanks @KKoovalsky for pointing this out.

I tried to use the alias function names define, and as best I can tell it didn't do anything. Looking at the code, it isn't clear to me how it's supposed to work. I am on the latest 27 Sep commit.

#ifdef PRINTF_ALIAS_STANDARD_FUNCTION_NAMES
# define printf_    printf_
# define sprintf_   sprintf_
# define vsprintf_  vsprintf_
# define snprintf_  snprintf_
# define vsnprintf_ vsnprintf_
# define vprintf_   vprintf_
#endif

// ... some stuff here ...

#ifdef PRINTF_ALIAS_STANDARD_FUNCTION_NAMES
# undef printf_
# undef sprintf_
# undef vsprintf_
# undef snprintf_
# undef vsnprintf_
# undef vprintf_
#endif

I temporarily hand-edited my local printf.h file and went back to way function name aliasing worked in the previous 17 Aug commit. This works, and I understand why it works (unlike the current commit).

#ifdef PRINTF_ALIAS_STANDARD_FUNCTION_NAMES
# define printf    printf_
# define sprintf   sprintf_
# define vsprintf  vsprintf_
# define snprintf  snprintf_
# define vsnprintf vsnprintf_
# define vprintf   vprintf_
#endif

// ... some stuff here ...

I did read the issue #14 notes, but they didn't explain what I'm experiencing. Regarding the 27 Sep commit, am I missing something about how to make it work and/or understanding how it should work?

Thanks,
-Chris

Allow the printf library to be used in host-side CUDA code (built with nvcc).

This probably doesn't need much real work, but - some warning-avoidance may be necessary, plus a test suite (maybe just replicate the existing one for starters?)

The C standard library printf()-family functions support an %a specifier, which takes a double value and prints it with perfect accuracy in exponent notation with hexadecimal digits. We should support this (although with an option to not-compile this support, like we have for other floating-point capabilities.

On AVR, and most likely some other 8-bit platforms, double is only 32bit.
This breaks %e and %g (but not %f).
Ideally, it would be nice to provide some kind of %e %g functionality when double is 32bit, perhaps with reduced precision.
At a minimum, a build error should occur if %e and or %g is enabled on these platforms.

By default, the API (defined in the README) is visible out of printf.c (and exposed in printf.h), and auxiliary functions are invisible (static). However, it is conceivable that users of this library may want:

• Everything static - for the case of #include "printf.c".
• the default described above
• Everything visible (extern) - when writing printf-ish code which can stand to use our innards (for some reason...)

we could support this, e.g. similarly to how nanoprintf does.

The C standard library printf()-family functions support a %n specifier, which takes a point to an integral type and updates it with the number of characters written so far for the format string. We do not yet support this, and while it's possible to achieve the same effect with fctprintf() and a nilpotent output function, we should implement actual support for this specifier.

Unfortunately (?), CUDA does not support global device-side constant data. And we happen to be using such data in our printf implementation: The powers-of-10 table. For now, let's apply a quick-and-perhaps-dirty fix: Make it a static __constant__ array inside a function for obtaining the power-of-10. Of course, for regular CPU execution, things basically stays the same (the function will just lookup a value in the global array, meaning it should be inline.

If we printf_("%#.1g", -40661.5), we currently get -40662, but should be getting -4.e+04 because of the low precision: With "%g", the "precision" value means the number of significant digits to show; and if the number isn't fully captured by those, we are supposed to use exponential notation.

If we disable support for long long specifiers (e.g. "%llu"), a bunch of test suite checks fail. These should mostly be removed, with one or a few checks for unsupported specifier being properly ignored.

This also relates to #45 , since some of the checks may involve pointers, and there we might actually have a correctness issue when long long support is turned off.

The test suite currently has some checks for expected results of "10e+2", "10e+5" etc. ; they're not exposed on my current system because they depend on the threshold of switching from decimal to exponential notation, but they're wrong.

printf.c: In function ‘sprint_exponential_number’:
printf.c:491:25: warning: ‘normalization.raw_factor’ may be used uninitialized in this function [-Wmaybe-uninitialized]
struct scaling_factor account_for_precision = update_normalization(normalization, prec_power_of_10);
^
printf.c:625:25: note: ‘normalization.raw_factor’ was declared here
struct scaling_factor normalization;

We currently use #cmakedefine in our printf_config.h.in file, but printf.c expects a different logic: It expects the value to always be defined, otherwise it chooses the default value (rather than interpreting undefined as false).

When setting the option ALIAS_STANDARD_FUNCTION_NAMES to ON only the definitions inside printf.c will be affected. Inside printf.c https://github.com/eyalroz/printf/blob/develop/src/printf/printf.c#L54:

#if PRINTF_ALIAS_STANDARD_FUNCTION_NAMES
# define printf_    printf
# define sprintf_   sprintf
# define vsprintf_  vsprintf
# define snprintf_  snprintf
# define vsnprintf_ vsnprintf
# define vprintf_   vprintf
#endif

But in the printf.h there is no include "printf_config.h", thus the declarations will never be aliased.

It means that, if the option is set to ON and the library is compiled, installed and consumed:

#include "printf.h"

int main()
{
    char c[64]; 
    snprintf(c, sizeof(c), "%d %f", 5, 160.1);

    return 0;
}

One gets compilation error:

main.cpp: In function ‘int main()’:
main.cpp:6:12: error: ‘snprintf’ was not declared in this scope
6 | snprintf(c, sizeof(c), "%d %f", 5, 160.1);
| ^~~~~~~~

Possible solutions:

1. Install printf_config.h along with printf.h and put #include "printf.h" at the top of printf.h.
2. Generate printf.h and rename the function names basing on the ALIAS_STANDARD_FUNCTION_NAMES option value.
3. Generate printf.h by putting PRINTF_ALIAS_STANDARD_FUNCTION_NAMES macro dynamically at the top of the printf.h file during installation and use target_compile_definitions(printf PUBLIC PRINTF_ALIAS_STANDARD_FUNCTION_NAMES) for the build interface.
4. Other?

There are a few issues with the compiler definitions:

• Some names I've chosen don't exactly fit the definition's actual semantics.
• I really dislike having a pair of definitions for enabling and disabling support for things ... so I think I'll switch to a boolean value which can be defined false (or rather 0) if necessary
• The CMakeLists.txt and printf.c explanatory comments should correspond better.
• The CMake option names and compiler definitions should correspond better.

... and this all will be a first phase to switching to a generated configuration file for building the library.

Hi,
I just noticed, there is typo in readme. Its Marco Paland. In fact, it's there several times

According to the C2X standard draft and the glibc documentation, we should avoid using identifiers starting with an underscore (_) visible at file scope. So, let's avoid that by renaming our _putchar() into putchar().

The printf_config.h file is not properly generated - we're not using #cmakedefine the right way, and we need to do some name prefix magic in the CMakeLists.txt file.

... Also, there's a typo in the definedness check for PRINTF_SUPPORT_PTRDIFF_LENGTH_MODIFIER in printf.c.

This test case fails compiling with MSVC 2017 in Win32:

https://en.cppreference.com/w/cpp/language/integer_literal

We often have one-off errors on the last digit of a number. Autotest output:

fmt = "% e" value = 9.622188e+03
gnu = " 9.622188e+03"
mpa = " 9.622187e+03"

fmt = "%+e" value = -4.136612e+04
gnu = "-4.136612e+04"
mpa = "-4.136613e+04"

fmt = "%e" value = 8.438362e+04
gnu = "8.438362e+04"
mpa = "8.438363e+04"

The autotest.cpp program used getopt.h, but that's not available on Windows.

For now, let's just bundle an available FOSS getopt implementation; but we should probably be able to drop the dependence on those extra ~650 lines-of-code (@mickjc750 ?)

My company aways use fixed int type . It's very disgusting to use "%" PRI64d "".

Hey,
I see that there is no CI server implemented at the moment, maybe this project needs one? If so, I'm willing to help you out. I have experience with the GitHub Actions (you can see my repos where I use Actions extensively MYREPO1, MYREPO2).

This definition is missing:

#define DISABLE_WARNING_PRINTF_FORMAT_EXTRA_ARGS
#define DISABLE_WARNING_PRINTF_FORMAT_OVERFLOW

for the _MSC_VER here:

example:
printf("%.3e", 9.9996)
produces "10.000e+00" which should be "1.000e+01"

Example:
%f is not available due to disabling PRINTF_SUPPORT_FLOAT_SPECIFIERS, and the application encounters a %f in a format string.
It would be useful to trap this so that the application can cause a runtime fault with an assertion handler.
This could be done with a macro which is ((void)0) by default, but could be edited in the config file to be something like ASSERT(false)

Let's streamline our CMake build a bit, with a printf_config.h file.

When built for Cortex-M3 using GCC with -Os, this option has almost no affect on the build size.
From minimal options (all features disabled). Enabling this option actually reduced build size by 16 bytes, somehow.

The C99 standard is vague about what exactly one is to pass for the "%zi" format specifier - it's a "signed size_t" - but that's not properly defined. We need to be explicit about what we expect for this specifier, so as not to confuse users.

If the library is "requested", via a CMake build option, to alias printf(), sprintf() etc. - Let's just do that with proper functions. Now, we can't "forward" variable argument functions except via va_list's but what we can do, and perhaps should do, is one of:

1. Define both printf_ and printf functions, with the same source code.
2. Use macros to choose between defining only printf_-style names or only printf-style names.

At the moment, we use many CHECK()s in our test suite. When a CHECK() fails, we get the string literal against with the test string was compared, but we don't know what's in that buffer compared against it, nor do we know which printing function had been invoked to generate it and with which arguments.

We should do something so that the additional information gets printed.

It would be useful to trap the event where the output becomes affected by the configured buffer sizes PRINTF_FTOA_BUFFER_SIZE or PRINTF_NTOA_BUFFER_SIZE.

This could be a macro in the printf_config.h which by default is ((void)0), but can be edited to something like WARN(false), depending on the applications runtime warning handler.

In commit 7a8444b, I introduced a naming inconsistency between a preprocessor define actually used and the define that's created by CMakeLists.txt ... rendering that CMakeLists.txt option useless. Thanks @KKoovalsky for drawing my attention to this in #56 .

Note: This also involves printf_config.h.in.

This wrapper exists, as a means of passing a function pointer and a null pointer, from the interface function all the way down to the character output function, which itself is a function pointer based on the output mode. This is needed to provide fctprintf().

A better approach, is to instantiate a variable set (struct) particular to the output mode in the interface function. Then pass this by reference through to the character output function.
This makes the character output function the same format (void (out)(char, void)) as that used by fctprintf().
This removes the need for a double function pointer de-reference per character which is a performance boost.
It also removes the need to pass the 'maxlen' parameter to most of the functions, where for most output modes it is unused.

The use of underscores in the source is excessive (IMHO).

As 'prn' is a common abbreviation of 'print', I suggest renaming printf to prnf.
This would avoid the need for most underscores, which are currently used to avoid name collisions with the standard library.
It would make it obvious in applications which use prnf(), that they are not using the standard implementation.
printf() could still be overridden by the prnf() implementation, using #defines as it is now.

The value is taken from va_arg initially here:

Compiling with MSVC (VS2017) this issue makes this test cases fail: