[libc] add fgets

[michaelrj]

Summary

This adds the fgets function and its unit tests.

[sivachandra]

libc/src/stdio/fgets.cpp

24

Nit: Because reinterpret_cast already shows the type, you use auto:

auto *stream = ...;

26

Use FileLock instead:

File::FileLock lock(stream);

You can use a nested block to reduce the scope of the lock.

libc/test/src/stdio/fgets_test.cpp

71

Add tests for error.

80

Reading more should still be an EOF and not an error you mean?

[michaelrj]

address comments

libc/src/stdio/fgets.cpp

26

I tried using FileLock but it's a private part of File.

libc/test/src/stdio/fgets_test.cpp

71

The only true error state for fgets is a read error, which is tested above when I try to read from the write-only file. If there are other error states you see that I don't test then I'll add cases for them, but I don't see any.

[sivachandra]

libc/src/stdio/fgets.cpp

26

Ok, never mind then.

libc/test/src/stdio/fgets_test.cpp

71

What I mean is to add:

ASSERT_EQ(__llvm_libc::ferror(file), 0);

inside of the for block above.

[michaelrj]

add ferror checking.

[michaelrj]

fix a few style issues

libc/config/linux/x86_64/entrypoints.txt

list(APPEND TARGET_LIBC_ENTRYPOINTS
     libc.src.stdio.clearerr_unlocked
     libc.src.stdio.fclose
     libc.src.stdio.flockfile
     libc.src.stdio.feof
     libc.src.stdio.feof_unlocked
     libc.src.stdio.ferror
     libc.src.stdio.ferror_unlocked
     libc.src.stdio.fgetc
+    libc.src.stdio.fgets
     libc.src.stdio.fflush
     libc.src.stdio.fopen
     libc.src.stdio.fputc
     libc.src.stdio.fputs
     libc.src.stdio.fopencookie
     libc.src.stdio.fread
     libc.src.stdio.fread_unlocked
     libc.src.stdio.fseek

libc/spec/stdc.td

HeaderSpec StdIO = HeaderSpec<
               [ArgSpec<FILEPtr>]
           >,
           FunctionSpec<
               "fgetc",
               RetValSpec<IntType>,
               [ArgSpec<FILEPtr>]
           >,
           FunctionSpec<
+              "fgets",
+              RetValSpec<CharPtr>,
+              [
+                ArgSpec<CharRestrictedPtr>,
+                ArgSpec<IntType>,
+                ArgSpec<FILERestrictedPtr>,
+              ]
+          >,
+          FunctionSpec<
               "fflush",
               RetValSpec<IntType>,
               [ArgSpec<FILEPtr>]
           >,
           FunctionSpec<
               "fopen",
               RetValSpec<FILEPtr>,
               [ArgSpec<ConstCharPtr>,

libc/src/stdio/CMakeLists.txt

HDRS
     fgetc.h
   DEPENDS
     libc.include.stdio
     libc.src.__support.File.file
     libc.src.__support.File.platform_file
 )
 
 add_entrypoint_object(
+  fgets
+  SRCS
+    fgets.cpp
+  HDRS
+    fgets.h
+  DEPENDS
+    libc.include.stdio
+    libc.src.__support.File.file
+    libc.src.__support.File.platform_file
+)
+
+add_entrypoint_object(
   fflush
   SRCS
     fflush.cpp
   HDRS
     fflush.h
   DEPENDS
     libc.include.stdio
     libc.src.__support.File.file

libc/src/stdio/fgets.h

+//===-- Implementation header of fgets --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_FGETS_H
+#define LLVM_LIBC_SRC_STDIO_FGETS_H
+
+#include <stdio.h>
+
+namespace __llvm_libc {
+
+char *fgets(char *__restrict str, int count, ::FILE *__restrict raw_stream);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_FGETS_H

libc/src/stdio/fgets.cpp

+//===-- Implementation of fgets -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/fgets.h"
+#include "src/__support/File/file.h"
+
+#include <stddef.h>
+#include <stdio.h>
+
+namespace __llvm_libc {
+
+LLVM_LIBC_FUNCTION(char *, fgets,
+                   (char *__restrict str, int count,
+                    ::FILE *__restrict raw_stream)) {
+  if (count < 1) {
+    return nullptr;
+  }
+  unsigned char c = '\0';
+  __llvm_libc::File *__restrict stream =
+      reinterpret_cast<__llvm_libc::File *__restrict>(raw_stream);
+  stream->lock();
+
+  // i is an int because it's frequently compared to count, which is also int.
+  int i = 0;
+
+  for (; i < (count - 1) && c != '\n'; ++i) {
+    size_t r = stream->read_unlocked(&c, 1);
+    if (r != 1) {
+      break;
+    }
+    str[i] = c;
+  }
+
+  bool has_error = stream->error_unlocked();
+  bool has_eof = stream->iseof_unlocked();
+  stream->unlock();
+
+  // If the requested read size makes no sense, an error occured, or no bytes
+  // were read due to an EOF, then return nullptr and don't write the null byte.
+  if (has_error || (i == 0 && has_eof)) {
+    return nullptr;
+  }
+
+  str[i] = '\0';
+  return str;
+}
+
+} // namespace __llvm_libc

libc/test/src/stdio/CMakeLists.txt

DEPENDS
     libc.src.stdio.fclose
     libc.src.stdio.feof
     libc.src.stdio.ferror
     libc.src.stdio.fgetc
     libc.src.stdio.fopen
     libc.src.stdio.fwrite
 )
 
+add_libc_unittest(
+  fgets_test
+  SUITE
+    libc_stdio_unittests
+  SRCS
+    fgets_test.cpp
+  DEPENDS
+    libc.include.errno
+    libc.include.stdio
+    libc.src.stdio.fclose
+    libc.src.stdio.feof
+    libc.src.stdio.ferror
+    libc.src.stdio.fgets
+    libc.src.stdio.fopen
+    libc.src.stdio.fwrite
+)
+
 add_subdirectory(printf_core)
 add_subdirectory(testdata)

libc/test/src/stdio/fgets_test.cpp

+//===-- Unittests for fgets -----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/fclose.h"
+#include "src/stdio/feof.h"
+#include "src/stdio/ferror.h"
+#include "src/stdio/fgets.h"
+#include "src/stdio/fopen.h"
+#include "src/stdio/fwrite.h"
+#include "utils/UnitTest/Test.h"
+
+#include <errno.h>
+#include <stdio.h>
+
+TEST(LlvmLibcFgetsTest, WriteAndReadCharacters) {
+  constexpr char FILENAME[] = "testdata/fgets.test";
+  ::FILE *file = __llvm_libc::fopen(FILENAME, "w");
+  ASSERT_FALSE(file == nullptr);
+  constexpr char CONTENT[] = "123456789\n"
+                             "1234567\n"
+                             "123456\n"
+                             "1";
+  constexpr size_t WRITE_SIZE = sizeof(CONTENT) - 1;
+
+  char buff[8];
+  char *output;
+
+  ASSERT_EQ(WRITE_SIZE, __llvm_libc::fwrite(CONTENT, 1, WRITE_SIZE, file));
+  // This is a write-only file so reads should fail.
+  ASSERT_TRUE(__llvm_libc::fgets(buff, 8, file) == nullptr);
+  // This is an error and not a real EOF.
+  ASSERT_EQ(__llvm_libc::feof(file), 0);
+  ASSERT_NE(__llvm_libc::ferror(file), 0);
+  errno = 0;
+
+  ASSERT_EQ(0, __llvm_libc::fclose(file));
+
+  file = __llvm_libc::fopen(FILENAME, "r");
+  ASSERT_FALSE(file == nullptr);
+
+  // If we request just 1 byte, it should return just a null byte and not
+  // advance the read head. This is implementation defined.
+  output = __llvm_libc::fgets(buff, 1, file);
+  ASSERT_TRUE(output == buff);
+  ASSERT_EQ(buff[0], '\0');
+  ASSERT_EQ(errno, 0);
+
+  // If we request less than 1 byte, it should do nothing and return nullptr.
+  // This is also implementation defined.
+  output = __llvm_libc::fgets(buff, 0, file);
+  ASSERT_TRUE(output == nullptr);
+
+  const char *output_arr[] = {
+      "1234567", "89\n", "1234567", "\n", "123456\n", "1",
+  };
+
+  constexpr size_t ARR_SIZE = sizeof(output_arr) / sizeof(char *);
+
+  for (size_t i = 0; i < ARR_SIZE; ++i) {
+    output = __llvm_libc::fgets(buff, 8, file);
+
+    // This pointer comparison is intentional, fgets should return a pointer to
+    // buff when it succeeds.
+    ASSERT_TRUE(output == buff);
+
+    EXPECT_STREQ(buff, output_arr[i]);
+  }
+
+  // This should have hit the end of the file, but that isn't an error unless it
+  // fails to read anything.
+  ASSERT_NE(__llvm_libc::feof(file), 0);
+  ASSERT_EQ(__llvm_libc::ferror(file), 0);
+  ASSERT_EQ(errno, 0);
+
+  // Reading more should cause an error.
+  output = __llvm_libc::fgets(buff, 8, file);
+  ASSERT_TRUE(output == nullptr);
+  ASSERT_NE(__llvm_libc::feof(file), 0);
+  ASSERT_EQ(errno, 0);
+
+  ASSERT_EQ(0, __llvm_libc::fclose(file));
+}