Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
885d874
Initial moves
vinay-deshmukh Dec 23, 2024
12f2ae0
Remove duplicate definition
vinay-deshmukh Dec 27, 2024
6236e59
Doesn't build but should in CI
vinay-deshmukh Dec 27, 2024
d12a8f5
Merge branch 'main' into vinay-issue-115394
vinay-deshmukh Dec 27, 2024
f3b8d7f
clang-format patch
vinay-deshmukh Dec 27, 2024
bfe6b86
Put overlay include last
vinay-deshmukh Dec 27, 2024
90ce1b2
Merge branch 'main' into vinay-issue-115394
vinay-deshmukh Dec 27, 2024
1f9e27c
Attempt to fix linker error
vinay-deshmukh Dec 28, 2024
1bd2181
Merge branch 'main' into vinay-issue-115394
vinay-deshmukh Dec 28, 2024
0abb5c3
try reorder
vinay-deshmukh Dec 28, 2024
13f418d
Move getc back to header
vinay-deshmukh Jan 8, 2025
d3c3329
Merge commit 'f9369cc' into vinay-issue-115394
vinay-deshmukh Jan 8, 2025
fe6f638
WIP ???
vinay-deshmukh Jan 14, 2025
cbe1ab0
it works with COMPILE_OPTIONS param
vinay-deshmukh Jan 18, 2025
09194c2
clean header file
vinay-deshmukh Jan 19, 2025
a459d11
use same logic as vfscanf_internal
vinay-deshmukh Jan 19, 2025
6144b00
avoid name lookup ambiguity
vinay-deshmukh Jan 19, 2025
97ae074
clang-format patch
vinay-deshmukh Jan 19, 2025
76f6a31
Remove reader.cpp
vinay-deshmukh Jan 19, 2025
12696d4
Merge remote-tracking branch 'upstream/main' into vinay-issue-115394
vinay-deshmukh Jan 19, 2025
b02a039
Possibly redundant dependency for GPU
vinay-deshmukh Jan 19, 2025
82ccee6
fix COMPILE_OPTIONS for vfscanf_internal
vinay-deshmukh Jan 19, 2025
6f14edc
Add the right guards
vinay-deshmukh Feb 1, 2025
5c45508
Merge branch 'main' into vinay-issue-115394
vinay-deshmukh Feb 17, 2025
4539ae8
Remove extra spaces
vinay-deshmukh Feb 20, 2025
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
1 change: 1 addition & 0 deletions libc/src/stdio/scanf_core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ add_object_library(
reader.h
DEPENDS
libc.src.__support.macros.attributes
libc.hdr.types.FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the dependency on libc.src.__support.File.file, as well as the flag controlling if it's using the system file. You'll also need to set up proper handling for the GPU like what vfscanf_internal has.

Suggested change
libc.hdr.types.FILE
libc.src.__support.File.file
libc.hdr.types.FILE
${use_system_file}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seem to be getting an error like (after adding the above change):

  The dependency target "libc.src.__support.File.file" of target
  "libc.src.stdio.scanf_core.reader" does not exist.

is that expected on a M1 mac?

using the build commands from:#115394 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, that's because we only support overlay mode on mac, and overlay mode doesn't have FILE. You'll need to add logic to only add libc.src.__support.File.file to the list of DEPENDS if LLVM_LIBC_FULL_BUILD is true.

Copy link
Contributor Author

@vinay-deshmukh vinay-deshmukh Jan 18, 2025

Choose a reason for hiding this comment

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

like what vfscanf_internal has.

So, I think there's a bug / typo in the CMake call there.

Instead of:

  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}
  )

it should be:

  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
    COMPILE_OPTIONS
      ${use_system_file}
  )

I think is what caused my build to have the bugs with macOS' system stdio.h macros

Link to code:

libc.src.__support.arg_list
${use_system_file}
)

I can add the COMPILE_OPTIONS in this PR if that's oK?

Copy link
Contributor

Choose a reason for hiding this comment

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

The COMPILE_OPTIONS shouldn't be necessary, because use_system_file already contains it, see: https://github.com/llvm/llvm-project/blob/main/libc/src/stdio/CMakeLists.txt#L30

)

add_object_library(
Expand Down
63 changes: 62 additions & 1 deletion libc/src/stdio/scanf_core/reader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,71 @@

#include "src/stdio/scanf_core/reader.h"
#include "src/__support/macros/config.h"

#include <stddef.h>

#include "src/__support/File/file.h"
#include "hdr/types/FILE.h"

namespace LIBC_NAMESPACE_DECL {
namespace scanf_core {

namespace internal {

#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 int getc(void *f) {
return LIBC_NAMESPACE::getc(reinterpret_cast<::FILE *>(f));
}

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

#elif !defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)

LIBC_INLINE int getc(void *f) {
unsigned char c;
auto result =
reinterpret_cast<LIBC_NAMESPACE::File *>(f)->read_unlocked(&c, 1);
size_t r = result.value;
if (result.has_error() || r != 1)
return '\0';

return c;
}

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

#else // defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)

// Since ungetc_unlocked isn't always available, we don't acquire the lock for
// system files.
LIBC_INLINE int getc(void *f) { return ::getc(reinterpret_cast<::FILE *>(f)); }

LIBC_INLINE void ungetc(int c, void *f) {
::ungetc(c, reinterpret_cast<::FILE *>(f));
}
#endif // LIBC_COPT_STDIO_USE_SYSTEM_FILE

} // namespace internal

char Reader::getc() {
++cur_chars_read;
if (rb != nullptr) {
char output = rb->buffer[rb->buff_cur];
++(rb->buff_cur);
return output;
}
// This should reset the buffer if applicable.
return static_cast<char>(internal::getc(input_stream));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this being in the header was intentional. That way the fast path (reading directly from the buffer) gets properly inlined into the readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, will move it back to header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to move the ungetc implementation to the header as well?

The reason is, now that the internal::getc is compiled based on the platform depending on which macro is defined, if we keep getc in .h but ungetc in .cpp that macro if/else chain will need to exist in two places over just one if it gets moved to .h

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving ungetc into the header should be fine.


void Reader::ungetc(char c) {
--cur_chars_read;
if (rb != nullptr && rb->buff_cur > 0) {
Expand All @@ -23,7 +83,8 @@ void Reader::ungetc(char c) {
--(rb->buff_cur);
return;
}
stream_ungetc(static_cast<int>(c), input_stream);
internal::ungetc(static_cast<int>(c), input_stream);
}

} // namespace scanf_core
} // namespace LIBC_NAMESPACE_DECL
27 changes: 3 additions & 24 deletions libc/src/stdio/scanf_core/reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
namespace LIBC_NAMESPACE_DECL {
namespace scanf_core {

using StreamGetc = int (*)(void *);
using StreamUngetc = void (*)(int, void *);

// This is intended to be either a raw string or a buffer syncronized with the
// file's internal buffer.
struct ReadBuffer {
Expand All @@ -29,38 +26,20 @@ struct ReadBuffer {

class Reader {
ReadBuffer *rb;

void *input_stream = nullptr;

// TODO: Remove these unnecessary function pointers
StreamGetc stream_getc = nullptr;
StreamUngetc stream_ungetc = nullptr;

size_t cur_chars_read = 0;

public:
// TODO: Set buff_len with a proper constant
LIBC_INLINE Reader(ReadBuffer *string_buffer) : rb(string_buffer) {}

LIBC_INLINE Reader(void *stream, StreamGetc stream_getc_in,
StreamUngetc stream_ungetc_in,
ReadBuffer *stream_buffer = nullptr)
: rb(stream_buffer), input_stream(stream), stream_getc(stream_getc_in),
stream_ungetc(stream_ungetc_in) {}
LIBC_INLINE Reader(void *stream, ReadBuffer *stream_buffer = nullptr)
: rb(stream_buffer), input_stream(stream) {}

// This returns the next character from the input and advances it by one
// character. When it hits the end of the string or file it returns '\0' to
// signal to stop parsing.
LIBC_INLINE char getc() {
++cur_chars_read;
if (rb != nullptr) {
char output = rb->buffer[rb->buff_cur];
++(rb->buff_cur);
return output;
}
// This should reset the buffer if applicable.
return static_cast<char>(stream_getc(input_stream));
}
char getc();

// This moves the input back by one character, placing c into the buffer if
// this is a file reader, else c is ignored.
Expand Down
31 changes: 1 addition & 30 deletions libc/src/stdio/scanf_core/vfscanf_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,6 @@ 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));
}

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)
Expand All @@ -58,21 +50,6 @@ LIBC_INLINE void funlockfile(FILE *f) {
reinterpret_cast<LIBC_NAMESPACE::File *>(f)->unlock();
}

LIBC_INLINE int getc(void *f) {
unsigned char c;
auto result =
reinterpret_cast<LIBC_NAMESPACE::File *>(f)->read_unlocked(&c, 1);
size_t r = result.value;
if (result.has_error() || r != 1)
return '\0';

return c;
}

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

LIBC_INLINE int ferror_unlocked(FILE *f) {
return reinterpret_cast<LIBC_NAMESPACE::File *>(f)->error_unlocked();
}
Expand All @@ -85,12 +62,6 @@ LIBC_INLINE void flockfile(::FILE *) { return; }

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

LIBC_INLINE int getc(void *f) { return ::getc(reinterpret_cast<::FILE *>(f)); }

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

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

#endif // LIBC_COPT_STDIO_USE_SYSTEM_FILE
Expand All @@ -103,7 +74,7 @@ LIBC_INLINE int vfscanf_internal(::FILE *__restrict stream,
const char *__restrict format,
internal::ArgList &args) {
internal::flockfile(stream);
scanf_core::Reader reader(stream, &internal::getc, internal::ungetc);
scanf_core::Reader reader(stream);
int retval = scanf_core::scanf_main(&reader, format, args);
if (retval == 0 && internal::ferror_unlocked(stream))
retval = EOF;
Expand Down
Loading