This is an archive of the discontinued LLVM Phabricator instance.

[libc] enable printf using system FILE
ClosedPublic

Authored by michaelrj on Mar 13 2023, 4:44 PM.

Details

Summary

The printf and fprintf implementations use our internal implementation
to improve performance when it's available, but this patch enables using
the public FILE API for overlay mode.

Diff Detail

Event Timeline

michaelrj created this revision.Mar 13 2023, 4:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 13 2023, 4:44 PM
michaelrj requested review of this revision.Mar 13 2023, 4:44 PM
lntue accepted this revision.Mar 13 2023, 8:09 PM
lntue added inline comments.
libc/src/stdio/printf_core/file_writer.cpp
2

Can this #include be outside of the namespaces?

libc/test/src/stdio/CMakeLists.txt
163

@sivachandra : I would assume the current bazel build is kind of equivalent to overlay mode. So do we need to add this flag to the bazel overlay?

This revision is now accepted and ready to land.Mar 13 2023, 8:09 PM
michaelrj marked an inline comment as done.Mar 15 2023, 10:46 AM
michaelrj added inline comments.
libc/test/src/stdio/CMakeLists.txt
163

the current bazel overlay doesn't have stdio yet, so this flag will be added after both this patch and the printf bazel patch (https://reviews.llvm.org/D146100) land.

clean up includes to address comments

sivachandra added inline comments.Mar 16 2023, 10:36 AM
libc/src/stdio/CMakeLists.txt
521

Use a more focused if:

list(APPEND printf_deps libc.src.__support.arg_list libc.src.stdio.printf_core.vfprintf_internal)
if(LLVM_LIBC_FULL_BUILD)
 list(APPEND printf_deps  libc.src.__support.File.file libc.src.__support.File.platform_file libc.src.__support.arg_list)
 set(printf_copts "COMPILE_OPTIONS -D<...>)
endif()

add_entrypoint_object(
  printf
  SRCS
    printf.cpp
  HDRS
    printf.h
  DEPENDS
    ${printf_deps}
  ${printf_copts}
)
libc/src/stdio/printf.cpp
14

s/PUBLIC_FILE/SYSTEM_FILE

32–33

Have you considered making vfprintf_internal a template function which takes a suitable FileWriter object? The goal should be to confine #ifdef to only one place in the code. Since only one of the templates will get instantiated, it will not lead to code size bloat.

libc/test/src/stdio/fprintf_test.cpp
22

s/file_test/printf_test

michaelrj updated this revision to Diff 505944.Mar 16 2023, 3:33 PM
michaelrj marked 3 inline comments as done.

simplify and clarify the conditionals

michaelrj added inline comments.Mar 16 2023, 3:44 PM
libc/src/stdio/printf.cpp
32–33

vfprintf_internal already doesn't have this #ifdef, this condition only changes where the stdout comes from. I've changed the code to clarify that.

sivachandra added inline comments.Mar 16 2023, 3:50 PM
libc/src/stdio/printf.cpp
32–33

Sorry, my comment here is more general. Right now, you have a #ifdef conditional here, and two more in file_writer.cpp and file_writer.h. Using appropriate templates, you should be able to limit the #ifdef to just this file.

libc/src/stdio/printf_core/CMakeLists.txt
127

Even this if should be modified like the one in the other file.

michaelrj marked 2 inline comments as done.

move vfprintf_internal and file_writer to templates

libc/src/stdio/printf.cpp
32–33

I can't limit it to just this file, since fprintf also needs to select the correct template, but it's just in the entrypoints now.

sivachandra added inline comments.Mar 21 2023, 10:25 AM
libc/src/stdio/fprintf.cpp
21

Nit: Use UpperCase names for internal types.

libc/src/stdio/printf.cpp
19

Make this a macro, say PRINTF_STDOUT. Otherwise, you are adding a new symbol and an indirection.

libc/src/stdio/printf_core/file_writer.h
89

You can call flockfile in the constructor and funlockfile in the destructor, and make this a call to fwrite_unlocked.

michaelrj updated this revision to Diff 507165.Mar 21 2023, 3:49 PM
michaelrj marked 5 inline comments as done.

address comments

sivachandra accepted this revision.Mar 22 2023, 1:12 PM
This revision was landed with ongoing or failed builds.Mar 23 2023, 9:56 AM
This revision was automatically updated to reflect the committed changes.
libc/src/stdio/printf_core/vfprintf_internal.h