[libc]: Clean up unnecessary function pointers in scanf (#121215)

Resolves #115394

1. Move definitions of cross-platform `getc` `ungetc` to `reader.h`.
2. Remove function pointer members to define them once per platform in
`.h`
3. Built in overlay mode in macOS m1
4. Remove `reader.cpp` as it's empty now


Also, full build doesn't yet build on macos m1 AFAIK

Author: vinay-deshmukh

libc/src/stdio/scanf_core/CMakeLists.txt

@@ -54,15 +54,26 @@ add_object_library(
     libc.src.__support.arg_list
 )
 
-add_object_library(
+if(LIBC_TARGET_OS_IS_GPU)
+add_header_library(
+  reader
+  HDRS
+    reader.h
+  DEPENDS
+    libc.src.__support.macros.attributes
+)
+elseif((TARGET libc.src.__support.File.file) OR (NOT LLVM_LIBC_FULL_BUILD))
+add_header_library(
   reader
-  SRCS
-    reader.cpp
   HDRS
     reader.h
   DEPENDS
     libc.src.__support.macros.attributes
+    libc.hdr.types.FILE
+    libc.src.__support.File.file
+  ${use_system_file}
 )
+endif()
 
 add_object_library(
   converter

libc/src/stdio/scanf_core/reader.cpp

@@ -9,15 +9,68 @@
 #ifndef LLVM_LIBC_SRC_STDIO_SCANF_CORE_READER_H
 #define LLVM_LIBC_SRC_STDIO_SCANF_CORE_READER_H
 
+#include "hdr/types/FILE.h"
+
+#ifndef LIBC_COPT_STDIO_USE_SYSTEM_FILE
+#include "src/__support/File/file.h"
+#endif
+
 #include "src/__support/macros/attributes.h" // For LIBC_INLINE
 #include "src/__support/macros/config.h"
+
 #include <stddef.h>
 
 namespace LIBC_NAMESPACE_DECL {
 namespace scanf_core {
-
-using StreamGetc = int (*)(void *);
-using StreamUngetc = void (*)(int, void *);
+// We use the name "reader_internal" over "internal" because
+// "internal" causes name lookups in files that include the current header to be
+// ambigious i.e. `internal::foo` in those files, will try to lookup in
+// `LIBC_NAMESPACE::scanf_core::internal` over `LIBC_NAMESPACE::internal` for
+// e.g., `internal::ArgList` in `libc/src/stdio/scanf_core/scanf_main.h`
+namespace reader_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 reader_internal
 
 // This is intended to be either a raw string or a buffer syncronized with the
 // file's internal buffer.
@@ -29,24 +82,15 @@ 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
@@ -59,12 +103,23 @@ class Reader {
       return output;
     }
     // This should reset the buffer if applicable.
-    return static_cast<char>(stream_getc(input_stream));
+    return static_cast<char>(reader_internal::getc(input_stream));
   }
 
   // This moves the input back by one character, placing c into the buffer if
   // this is a file reader, else c is ignored.
-  void ungetc(char c);
+  LIBC_INLINE void ungetc(char c) {
+    --cur_chars_read;
+    if (rb != nullptr && rb->buff_cur > 0) {
+      // While technically c should be written back to the buffer, in scanf we
+      // always write the character that was already there. Additionally, the
+      // buffer is most likely to contain a string that isn't part of a file,
+      // which may not be writable.
+      --(rb->buff_cur);
+      return;
+    }
+    reader_internal::ungetc(static_cast<int>(c), input_stream);
+  }
 
   LIBC_INLINE size_t chars_read() { return cur_chars_read; }
 };

libc/src/stdio/scanf_core/reader.h

@@ -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)
@@ -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();
 }
@@ -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
@@ -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;