Skip to content

[libc] Add scanf support to the GPU build #104812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 21, 2024
Merged

[libc] Add scanf support to the GPU build #104812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff7dcb8
[libc] Add `scanf` support to the GPU build
jhuber6 Aug 19, 2024
d1bc5fc
Update vfscanf_internal.h
jhuber6 Aug 20, 2024
ee2831d
Update vfscanf_internal.h
jhuber6 Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libc/config/gpu/entrypoints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ set(TARGET_LIBC_ENTRYPOINTS
libc.src.stdio.vsprintf
libc.src.stdio.asprintf
libc.src.stdio.vasprintf
libc.src.stdio.scanf
libc.src.stdio.fscanf
libc.src.stdio.sscanf
libc.src.stdio.vsscanf
libc.src.stdio.feof
Expand Down
2 changes: 2 additions & 0 deletions libc/docs/gpu/support.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ snprintf |check|
vsprintf |check|
vsnprintf |check|
sscanf |check|
scanf |check|
fscanf |check|
putchar |check| |check|
fclose |check| |check|
fopen |check| |check|
Expand Down
2 changes: 1 addition & 1 deletion libc/src/stdio/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ list(APPEND scanf_deps
libc.hdr.types.FILE
)

if(LLVM_LIBC_FULL_BUILD)
if(LLVM_LIBC_FULL_BUILD AND NOT LIBC_TARGET_OS_IS_GPU)
list(APPEND scanf_deps
libc.src.__support.File.file
libc.src.__support.File.platform_file
Expand Down
46 changes: 29 additions & 17 deletions libc/src/stdio/scanf_core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,21 +92,33 @@ add_object_library(
libc.src.__support.str_to_float
)

if(NOT (TARGET libc.src.__support.File.file) AND LLVM_LIBC_FULL_BUILD)
# Not all platforms have a file implementation. If file is unvailable, and a
# full build is requested, then we must skip all file based printf sections.
return()
if(LIBC_TARGET_OS_IS_GPU)
add_header_library(
vfscanf_internal
HDRS
vfscanf_internal.h
DEPENDS
.reader
.scanf_main
libc.include.stdio
libc.src.__support.arg_list
libc.src.stdio.getc
libc.src.stdio.ungetc
libc.src.stdio.ferror
COMPILE_OPTIONS
-DLIBC_COPT_STDIO_USE_SYSTEM_FILE
)
elseif(TARGET libc.src.__support.File.file OR (NOT LLVM_LIBC_FULL_BUILD))
add_header_library(
vfscanf_internal
HDRS
vfscanf_internal.h
DEPENDS
.reader
.scanf_main
libc.include.stdio
libc.src.__support.File.file
libc.src.__support.arg_list
${use_system_file}
)
endif()

add_header_library(
vfscanf_internal
HDRS
vfscanf_internal.h
DEPENDS
.reader
.scanf_main
libc.include.stdio
libc.src.__support.File.file
libc.src.__support.arg_list
${use_system_file}
)
28 changes: 27 additions & 1 deletion libc/src/stdio/scanf_core/vfscanf_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,43 @@
#include "src/__support/File/file.h"
#include "src/__support/arg_list.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/properties/architectures.h"
#include "src/stdio/scanf_core/reader.h"
#include "src/stdio/scanf_core/scanf_main.h"

#if defined(LIBC_TARGET_ARCH_IS_GPU)
#include "src/stdio/ferror.h"
#include "src/stdio/getc.h"
#include "src/stdio/ungetc.h"
#endif

#include "hdr/types/FILE.h"
#include <stddef.h>

namespace LIBC_NAMESPACE_DECL {

namespace internal {

#ifndef LIBC_COPT_STDIO_USE_SYSTEM_FILE
#if defined(LIBC_TARGET_ARCH_IS_GPU)
// The GPU build provides FILE access through the host operating system's
// library. So here we simply use the public entrypoints like in the SYSTEM_FILE
// interface. Entrypoints should normally not call others, this is an exception.
// FIXME: We do not acquire any locks here, so this is not thread safe.
LIBC_INLINE void flockfile(::FILE *) { return; }

LIBC_INLINE void funlockfile(::FILE *) { return; }

LIBC_INLINE int getc(void *f) {
return LIBC_NAMESPACE::getc(reinterpret_cast<::FILE *>(f));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're intentionally using the public entrypoints here, you need to add a specific comment explaining why it's important in this case, since we generally don't allow that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it's alright since it's the namespace qualified one, but not doing this would require a bit of a mess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the namespace isn't the issue, it's the fact that it makes entrypoints less independent. If entrypoints include each other directly then you have to include them as a group, you can't just get scanf without ungetc. This causes problems when trying to rollout individual functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought that's what dependencies were for, but I did add a comment about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependencies mean the build won't break, but that doesn't help with the rollout problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with leaving it as just a comment, the thing I want is to make it clear that this is unusual and people shouldn't copy it. I'm willing to let the GPU functions be less independent since there's no existing libc to overlay, but I want to make sure the functions on other targets stay independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a bit extra

}

LIBC_INLINE void ungetc(int c, void *f) {
LIBC_NAMESPACE::ungetc(c, reinterpret_cast<::FILE *>(f));
}

LIBC_INLINE int ferror_unlocked(::FILE *f) { return LIBC_NAMESPACE::ferror(f); }

#elif !defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)

LIBC_INLINE void flockfile(FILE *f) {
reinterpret_cast<LIBC_NAMESPACE::File *>(f)->lock();
Expand Down
Loading