This is an archive of the discontinued LLVM Phabricator instance.

[libc] add printf writer
ClosedPublic

Authored by michaelrj on Apr 25 2022, 3:48 PM.

Details

Summary

The printf implmentation is made up of three main pieces, the parser,
the converter, and the writer. This patch adds the implementation for
the writer, as well as the function for writing to a string, along with
tests.

Diff Detail

Event Timeline

michaelrj created this revision.Apr 25 2022, 3:48 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 25 2022, 3:48 PM
michaelrj requested review of this revision.Apr 25 2022, 3:48 PM
lntue accepted this revision.Apr 26 2022, 3:20 PM
lntue added inline comments.
libc/src/stdio/printf_core/writer.cpp
23

the check chars_written + length >= max_length is redundant. You can move it to an inline comment as reminding.

This revision is now accepted and ready to land.Apr 26 2022, 3:20 PM
michaelrj updated this revision to Diff 425328.Apr 26 2022, 4:08 PM
michaelrj marked an inline comment as done.

address comment to remove redundant condition

sivachandra added inline comments.Apr 27 2022, 2:47 PM
libc/src/stdio/printf_core/CMakeLists.txt
28

Indent.

libc/src/stdio/printf_core/string_writer.h
24

It seems to me like you want a stateful writer. So, why not just make it a class?

libc/src/stdio/printf_core/writer.h
31

Can you explain the need for max_length? It seems to me like it is something a specific writer, say StringWriter should care about, but not a generic writer?

At this point, I would actually ask the need for a generic Writer class. It seems to me like all you need is a writer class which implements a method with a specific signature. For example, you want StringWriter like this:

class StringWriter {
private:
  ... // State as suggested in another comment
public:

  // This method attempts to write |len| bytes from |data| and return the actual
  // number of bytes written.
  size_t write(const void *data, size_t len);
};

FWIW, the File class already implements the write protocol. So, based on the printf function, sprintf or fprintf, one either uses the StringWriter or File.

33

Is this required only for testing or do you need it for something else also?

sivachandra added inline comments.Apr 28 2022, 1:00 AM
libc/src/stdio/printf_core/writer.h
31

Actually, I am beginning think there is benefit with the generic Writer indirection. Consider the File example again - file writer might want to explicitly lock and use unlocked file methods to write to the file. Having a generic writer allows us to hide such details in the file writer.

michaelrj marked 4 inline comments as done.

add StringWriter class and adjust tests to match

libc/src/stdio/printf_core/string_writer.h
24

it felt like overkill for tracking only one variable, but if I'm moving max_len in there as well, then it makes more sense.

libc/src/stdio/printf_core/writer.h
31

File doesn't support all of the functions I need, specifically it doesn't have chars_written, which represents the number of characters written by this specific call to printf and is used both in the %n conversion and as the return value for all printf functions.

I have moved max_length into the StringWriter class now though.

31

I agree, and theoretically on platforms that don't care as much about code size we could have two string writer versions, one with a max length and one without, since that would improve performance.

33

chars_written is the return value of printf, as well as the value to be written for %n

sivachandra added inline comments.May 2 2022, 10:56 AM
libc/src/stdio/printf_core/string_writer.h
21

Nit: May be call it available_capacity as that is how it is being used internally?

27

Nit: Why not s/init_output/buffer and call the class member as buffer_ptr?

28

Nit: Space between line 27 and 28.

libc/src/stdio/printf_core/writer.cpp
21

The + operator can potentially lead to a rollover so the check should be done the other way around?

Couple of other points:

  1. It seems like all this logic redundant? For, the specific writer will/should handle this anyway?
  2. max_length of 0 is also handled by the specific writer anyway?

May be the raw_write should be a "raw" writer anyway. As in, it will just do the writing business and all higher level logic is up to this Writer class.

28

The + operation here can also rollover. Is it OK?

michaelrj updated this revision to Diff 426474.May 2 2022, 11:59 AM
michaelrj marked 6 inline comments as done.

fix max_length still being in the core writer class, and the tests not using the max_length functionality in the string writer.

libc/src/stdio/printf_core/string_writer.h
27

I went with cur_buffer since I felt that more accurately described how it was being used.

libc/src/stdio/printf_core/writer.cpp
21

The max length here should have been removed in the previous patch because I moved the max length logic to the string writer class, so yes it was redundant. As for if raw_write should be relatively logic free, I think it makes sense to have the length handled in the string writer, since the max length is only used for strings. Other than that it is relatively raw, only storing a pointer and the number of bytes left to write to it.

28

chars_written is supposed to store the value that will be returned when printf completes. The return value of printf is the number of characters written by that call as represented by an int, or a negative number if there was an error. For this purpose, all chars_written needs to do is be at least as large as an int, and if there would be overflow, return a negative number to represent the error. For this purpose I've changed chars_written to an unsigned long long so that it will definitely be bigger than an int, and the actual printf code can return a negative number if it would overflow that int. Finally, unsigned long long is guaranteed to have a maximum value of at least 18446744073709551615 (2^64 - 1), and if any call to printf writes more than that many bytes I'm declaring it undefined behavior.

sivachandra accepted this revision.May 2 2022, 2:05 PM
sivachandra added inline comments.
libc/src/stdio/printf_core/string_writer.h
43

I suppose this method will be used by s*printf functions explicitly?

libc/src/stdio/printf_core/writer.cpp
28

heh!

lntue accepted this revision.May 3 2022, 9:36 AM
michaelrj added inline comments.May 3 2022, 10:14 AM
libc/src/stdio/printf_core/string_writer.h
43

yes.

This revision was automatically updated to reflect the committed changes.