This is an archive of the discontinued LLVM Phabricator instance.

[libc] add printf base 10 integer conversion
ClosedPublic

Authored by michaelrj on May 18 2022, 2:10 PM.

Details

Summary

This patch adds support for d, i, and u conversions in printf, as well
as comprehensive unit tests.

Diff Detail

Event Timeline

michaelrj created this revision.May 18 2022, 2:10 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 18 2022, 2:10 PM
michaelrj requested review of this revision.May 18 2022, 2:10 PM
michaelrj updated this revision to Diff 430523.May 18 2022, 3:59 PM

add sprintf integer tests

sivachandra added inline comments.May 19 2022, 1:41 PM
libc/src/stdio/printf_core/int_converter.h
25

Seems like this one line function is used only once?

39

BUFF_LEN

40

Can this assignment ever lead to a truncation? May no because this is a convert_int which does not deal with integers wider than 64-bit?

60

Why are these masking operations required? Also, for max values, we have: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/Limits.h

91
for (; num > 0; --buff_cur, num /= 10)
  buffer[buff_curr] = (num % 10) + '0';
michaelrj updated this revision to Diff 430803.May 19 2022, 2:08 PM
michaelrj marked 4 inline comments as done.

address comments

michaelrj added inline comments.May 20 2022, 10:33 AM
libc/src/stdio/printf_core/int_converter.h
40

It can't, because uintmax_t is at least as large as the largest valid integer type for printf. The size of conv_val_raw may be larger than uintmax_t to fit long double values, but the value passed by the user can't be larger than uintmax_t.

60

This is to handle the fact that the length modifiers allow the user to specify the size of the integer to be converted. The type in num is always uintmax_t since that can fit any valid type, but the user can specify that they want it to be treated as a short, in which case we have to limit the value of num to what can fit in a short. The way this is done here is by creating a mask of just the bits in the specified type.

sivachandra added inline comments.May 20 2022, 11:16 AM
libc/src/stdio/printf_core/int_converter.h
60

Thanks for explaining. You can either use LLVM libc's own NumericLimits or directly use macros from the freestanding limits.h header. NumericLimits is probably more appropriate as you can get max values of size_t and ptrdiff_t is a straightforward way.

michaelrj updated this revision to Diff 434597.Jun 6 2022, 1:47 PM

switch to using limits.h for type size masks, additionally adjust the buffer size to be more exact.

michaelrj marked 2 inline comments as done.Jun 6 2022, 1:47 PM
sivachandra accepted this revision.Jun 7 2022, 11:58 AM

LGTM - But, if some of the changes to the tests are unrelated, then do it separately.

libc/src/stdio/printf_core/int_converter.h
19

Does this have to be in a header file? If yes, then we should mark it inline and also add header guards. You should probably do that with other <converter>.h files?

41

Nit: Do you need the static cast?

127

Nit: The first word starts with a captilized letter. Can we follow the same convention with all comments please?

libc/test/src/stdio/printf_core/converter_test.cpp
27

I am probably missing something - how are these changes to string conversion tests related to integer conversion? Also, do we really want a separate string converter and writer for every test? If not, they can be static globals.

Another point, which can be addressed separately - we should probably not be testing against internal API for a converter, writer etc. You can have a test for verifying that the plumbing works, but everything else should probably be tested using sprintf.

185

Should we put these integer conversion tests in a separate file?

This revision is now accepted and ready to land.Jun 7 2022, 11:58 AM
sivachandra added inline comments.Jun 7 2022, 12:04 PM
libc/test/src/stdio/printf_core/converter_test.cpp
27

I forgot that StringWriter is stateful - ignore that part of the comment about needing a new string writer for every test.

michaelrj updated this revision to Diff 435325.Jun 8 2022, 2:21 PM
michaelrj marked 5 inline comments as done.

move tests into sprintf_tests and split converter_test changes into a separate patch

libc/src/stdio/printf_core/int_converter.h
41

yes. The output of the arithmetic and is an int which doesn't have a default conversion to FormatFlags

libc/test/src/stdio/printf_core/converter_test.cpp
27

I think that having some tests specific to the writer, parser, and converter is helpful for determining which piece has the bug, although we can move primary functionality testing to sprintf for simplicity.

sivachandra accepted this revision.Jun 8 2022, 10:01 PM

Feel free to submit after addressing the comments.

libc/src/stdio/printf_core/int_converter.h
41

Ah, I meant to ask if you can use FormatFlags(...) without the static_cast.

libc/src/stdio/printf_core/string_converter.h
8 ↗(On Diff #435325)

Header guards?

michaelrj updated this revision to Diff 435600.Jun 9 2022, 10:16 AM
michaelrj marked 2 inline comments as done.

address comments and add simple converter test

libc/src/stdio/printf_core/string_converter.h
8 ↗(On Diff #435325)

yes, I meant to add those and forgot.

This revision was automatically updated to reflect the committed changes.