[libc][bazel] add file printf targets and support

[michaelrj]

Summary

This patch adds targets for printf and fprintf to the bazel build.
Additionally, it adds support for the build system to specify where
files should be written for testing purposes. This was necessary to
enable the fprintf test under bazel.

[michaelrj]

add DISABLE_INDEX_MODE and DISABLE_WRITE_INT flags, unify the PRINTF_COPTS location, and adjust the libc_function rule to properly handle immutable lists.

[sivachandra]

To avoid calling getenv on all platforms, I suggest doing this:

1. Add a convenience function declaration, say cpp::string make_test_file_path(const char *) in LibcTest.h.
2. Add implementations in cmake.cpp and bazel.cpp for CMake and Bazel restpectively. The one in cmake.cpp would just be a passthrough. The one in bazel.cpp will implement mostly the logic you have added in this patch.
3. In the CMake target for LibcUnitTest add cmake.cpp and in the one for Bazel, add bazel.cpp.

[michaelrj]

rewrite to make the test file path system more generic.

[sivachandra]

libc/test/UnitTest/BazelFilePath.cpp

19 ↗

(On Diff #511557)

We should ideally avoid depending on global object initialization. Can we call getenv from inside the function below?

libc/test/UnitTest/CmakeFilePath.cpp

20 ↗

(On Diff #511557)

Do we really need the testdata/ prefix? As in, can we not just create a file in the same directory as the test? It makes this function a simple pass through.

libc/utils/testutils/OwnedString.h

14 ↗

(On Diff #511557)

Why is this class required?

utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl

13 ↗

(On Diff #511557)

These should be defined in libc/BUILD.bazel. Also, they should passed to libc_function as defines = PRINTF_COPTS, which then will be passed to tests which depend on those functions which use those defines.

utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl

17

Unused?

[michaelrj]

address comments and move OwnedString to src/__support/CPP/

libc/utils/testutils/OwnedString.h

14 ↗

(On Diff #511557)

This class holds the cpp::string so that it doesn't get deallocated, and is implicitly castable to char *.

In the test, the pattern is auto FILE_PATH = libc_make_test_file_path("filepath");, and then the FILE_PATH can be used as a char *. The actual type of FILE_PATH might be char* or OwnedString, but both work as char*.
We can't just have FILE_PATH be a cpp::string because cpp::string isn't implicitly castable to char*, and I'm trying to avoid needing a static cast every time the FILE_PATH is used.
Similarly, the FILE_PATH can't just be a char* since if we don't store the string it gets deallocated.
The original intent of this class was for FILE_PATH to always be an OwnedString, which is why it's a template, but that's not necessary now.

I've also moved this to __support/CPP/ since LibcTest.h says to only include headers from that folder and testutils.

[sivachandra]

libc/src/__support/CPP/string.h

116 ↗

(On Diff #512969)

Is this part of std::string, with the explicit tag?

libc/utils/testutils/OwnedString.h

14 ↗

(On Diff #511557)

So, two things you say:

1. Doesn't get deallocated.
2. Implicitly castable to char *.

I get #2 but can you explain #1 more?

[michaelrj]

respond to comments

libc/src/__support/CPP/string.h

116 ↗

(On Diff #512969)

no, I added it when I thought OwnedString would need to be templated so that both cpp::string and const char* arguments could be safely handled in the same way. I've removed it.

libc/utils/testutils/OwnedString.h

14 ↗

(On Diff #511557)

By "Doesn't get deallocated" I mean that if we made FILE_PATH a const char* and static cast the return value from libc_make_test_file_path to const char* then the cpp::string that libc_make_test_file_path returns would not be stored. The value of FILE_PATH would be set to the pointer to the buffer inside the cpp::string, then the cpp::string would be destructed, freeing that memory.

OwnedString solves this problem by owning the cpp::string and its memory, keeping it from being destructed until the end of the function.

[sivachandra]

libc/src/__support/CPP/OwnedString.h

1 ↗

(On Diff #515070)

This class should live outside of the CPP directory.

19 ↗

(On Diff #515070)

You can make this a template class like this:

template <typename T>
class CString {
  T str;
public:
  // These constructors can be implemented iff required.
  CString() = delete;
  CString(const CString &) = delete;
  CString(CString &&) = delete;

  LIBC_INLINE CString(const T &s) : str(s) {}
  LIBC_INLINE operator const char *() const { return str.data(); }
};

template <>
class CString<const char *> {
  const char *str;
public:
  // These constructors can be implemented iff required.
  CString() = delete;
  CString(const CString &) = delete;
  CString(CString &&) = delete;

  LIBC_INLINE CString(const char *s) : s(str) {}
  LIBC_INLINE operator const char *() const { return str; }
};

The template will work with cpp::string_view and cpp::span also.

libc/test/src/stdio/fprintf_test.cpp

40

This comment is for the OwnedString de-allocation problem you have explained.

This is working for you because FILE_PATH is actually a variable and not a temporary. Had you used like,

const char *FILE_PATH = libc_make_test_file_path(FILENAME);
::FILE *file = printf_test::fopen(FILE_PATH, "w");

on line 40, you would had the same de-allocation problem. Put other way around, if you make FILE_PATH a cpp::string instance, de-allocation will not happen until the end of this test's scope. The ability to implicitly convert it to a const char * value is still desirable, so I have added a comment under OwnedString.

[michaelrj]

address comments

libc/src/__support/CPP/OwnedString.h

1 ↗

(On Diff #515070)

I can't move it out of CPP because it's included from LibcTest.h and LibcTest.h specifies that it can only include headers from TestUtils and CPP.

19 ↗

(On Diff #515070)

I can't make this class a template because the function it's returning from would need to know the template arguments, since they can't be deduced in a function return type. Both the Bazel and CMake version of the function would therefore need to be the same template type.

[sivachandra]

libc/src/__support/CPP/OwnedString.h

1 ↗

(On Diff #515070)

That comment should be fixed. The previous testutils now live in __support.

libc/src/__support/CPP/cstring.h

18 ↗

(On Diff #516472)

Is this comment correct?

31 ↗

(On Diff #516472)

Whats the idea behind keeping a cstr member?

[michaelrj]

address comments

libc/src/__support/CPP/cstring.h

18 ↗

(On Diff #516472)

I think so. This class is primarily useful as a read-only string that may be dynamically allocated, but will automatically clean itself up. The primary use for that is as a return value.

31 ↗

(On Diff #516472)

the cstr member is used below in the const char* initializer. It allows this to hold either a cpp::string or a const char*.

[sivachandra]

libc/src/__support/cstring.h

21 ↗

(On Diff #516551)

The comment trail is on a different diff, but let me restart it here.

When you have:

cpp::string str("abc");

The var str holds a copy of "abc". Which means that str can be destroyed safely without worrying about the cpp::string's desctructor trying to free its resources. So, you shouldn't need cstr and also not need the constructor on line 34. If you have code like this:

CString str("abc");

The constructor on line 29 will be called as const char * is implicitly convertible to cpp::string: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/string.h#L61.

The reason I suggested a template is because, in a general case, we should not try to copy const char * strings. You can do that adjustment later.

29 ↗

(On Diff #516551)

You should use LIBC_INLINE for code in src/

[michaelrj]

libc/src/__support/cstring.h

21 ↗

(On Diff #516551)

In my testing I'm not seeing the behavior you're describing. When I debug fprintf_test.cpp and look at which constructor CmakeFilePath.cpp is calling I see it using the one on line 34. I think what's happening is the compiler sees that the only call to CString is with a const char* and the version on line 29 is getting removed as unused from the final code since it's all inlined. Regardless, it's not constructing a non-trivial cpp::string.

[michaelrj]

move to LIBC_INLINE

[sivachandra]

libc/src/__support/cstring.h

21 ↗

(On Diff #516551)

In my testing I'm not seeing the behavior you're describing. When I debug fprintf_test.cpp and look at which constructor CmakeFilePath.cpp is calling I see it using the one on line 34.

That is because you have that constructor. Are you facing any problem if you remove that constructor?

[michaelrj]

remove unnecessary char* initializer for cstring and rename the header to c_string.h to clarify that this isn't related to cstring (the libc++ header).

libc/src/stdio/fprintf.cpp

 //===-- Implementation of fprintf -------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/stdio/fprintf.h"
 
-#include "src/__support/File/file.h"
 #include "src/__support/arg_list.h"
 #include "src/stdio/printf_core/vfprintf_internal.h"
 
 #include <stdarg.h>
 #include <stdio.h>
 
 namespace __llvm_libc {
 
 #ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+#include "src/__support/File/file.h"
 using FileT = __llvm_libc::File;
 #else  // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE)
 using FileT = ::FILE;
 #endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 
 LLVM_LIBC_FUNCTION(int, fprintf,
                    (::FILE *__restrict stream, const char *__restrict format,
                     ...)) {

libc/test/src/stdio/CMakeLists.txt

 add_libc_testsuite(libc_stdio_unittests)
 
+add_header_library(
+  testdata_path_util
+  HDRS
+    testdata_path_util.h
+  DEPENDS
+    libc.include.stdlib
+)
+
 add_libc_unittest(
   fileop_test
   SUITE
     libc_stdio_unittests
   SRCS
     fileop_test.cpp
   DEPENDS
     libc.include.stdio

libc/test/src/stdio/fprintf_test.cpp

 #include "src/stdio/fopen.h"
 #include "src/stdio/fread.h"
 #endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 
 #include "src/stdio/fprintf.h"
 
 #include "test/UnitTest/Test.h"
 
+#include "test/src/stdio/testdata_path_util.h"
+
 #include <stdio.h>
 
 namespace printf_test {
 #ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 using __llvm_libc::fclose;
 using __llvm_libc::ferror;
 using __llvm_libc::fopen;
 using __llvm_libc::fread;
 #else  // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE)
 using ::fclose;
 using ::ferror;
 using ::fopen;
 using ::fread;
 #endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 } // namespace printf_test
 
 TEST(LlvmLibcFPrintfTest, WriteToFile) {
-  constexpr char FILENAME[] = "testdata/fprintf_output.test";
-  ::FILE *file = printf_test::fopen(FILENAME, "w");
+  constexpr char FILENAME[] = "fprintf_output.test";
+  char FILE_PATH[1024] = {'\0'};
+
+  write_file_path(FILE_PATH, FILENAME);
+
+  ::FILE *file = printf_test::fopen(FILE_PATH, "w");
   ASSERT_FALSE(file == nullptr);
 
   int written;
 
   constexpr char simple[] = "A simple string with no conversions.\n";
   written = __llvm_libc::fprintf(file, simple);
   EXPECT_EQ(written, 37);
 
   constexpr char numbers[] = "1234567890\n";
   written = __llvm_libc::fprintf(file, "%s", numbers);
   EXPECT_EQ(written, 11);
 
   constexpr char format_more[] = "%s and more\n";
   constexpr char short_numbers[] = "1234";
   written = __llvm_libc::fprintf(file, format_more, short_numbers);
   EXPECT_EQ(written, 14);
 
   ASSERT_EQ(0, printf_test::fclose(file));
 
-  file = printf_test::fopen(FILENAME, "r");
+  file = printf_test::fopen(FILE_PATH, "r");
   ASSERT_FALSE(file == nullptr);
 
   char data[50];
   ASSERT_EQ(printf_test::fread(data, 1, sizeof(simple) - 1, file),
             sizeof(simple) - 1);
   data[sizeof(simple) - 1] = '\0';
   ASSERT_STREQ(data, simple);
   ASSERT_EQ(printf_test::fread(data, 1, sizeof(numbers) - 1, file),

libc/test/src/stdio/testdata_path_util.h

+//===-- Utilities for finding the testdata path ---------------------------===//
+//
+// 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 <stddef.h>
+#include <stdlib.h>
+
+// This is to support building with Bazel.
+static const char *const UNDECLARED_OUTPUTS_PATH =
+    getenv("TEST_UNDECLARED_OUTPUTS_DIR");
+
+constexpr char DEFAULT_DIR[] = "testdata/";
+
+inline void concat_path(char *output, const char *path, const char *filename) {
+  size_t i = 0;
+  for (; path[i] != '\0'; ++i) {
+    output[i] = path[i];
+  }
+  for (size_t j = 0; filename[j] != '\0'; ++i, ++j) {
+    output[i] = filename[j];
+  }
+}
+
+inline void write_file_path(char *output, const char *filename) {
+  if (UNDECLARED_OUTPUTS_PATH == nullptr ||
+      UNDECLARED_OUTPUTS_PATH[0] == '\0') {
+    concat_path(output, DEFAULT_DIR, filename);
+  } else {
+    concat_path(output, UNDECLARED_OUTPUTS_PATH, filename);
+  }
+}

utils/bazel/llvm-project-overlay/libc/BUILD.bazel

libc_support_library(
     hdrs = ["src/__support/CPP/cstddef.h"],
     deps = [
         ":__support_cpp_type_traits",
         ":libc_root",
     ],
 )
 
 libc_support_library(
+    name = "__support_cpp_expected",
+    hdrs = ["src/__support/CPP/expected.h"],
+    deps = [
+        ":libc_root",
+    ],
+)
+
+libc_support_library(
     name = "__support_cpp_functional",
     hdrs = ["src/__support/CPP/functional.h"],
     deps = [":libc_root"],
 )
 
 libc_support_library(
     name = "__support_cpp_limits",
     hdrs = ["src/__support/CPP/limits.h"],
     deps = [":libc_root"],
 )
 
+
+libc_support_library(
+    name = "__support_cpp_new",
+    hdrs = ["src/__support/CPP/new.h"],
+    srcs = ["src/__support/CPP/new.cpp"],
+    deps = [
+        ":libc_root",
+        ":__support_common",
+    ],
+)
+
 libc_support_library(
     name = "__support_cpp_optional",
     hdrs = ["src/__support/CPP/optional.h"],
     deps = [":libc_root"],
 )
 
 libc_support_library(
     name = "__support_cpp_span",
libc_support_library(
     hdrs = ["src/__support/arg_list.h"],
     deps = [
         ":libc_root",
         ":__support_common",
     ],
 )
 
 libc_support_library(
+    name = "__support_error_or",
+    hdrs = ["src/__support/error_or.h"],
+    deps = [
+        ":libc_root",
+        ":__support_cpp_expected",
+        ":__support_common",
+    ],
+)
+
+libc_support_library(
     name = "__support_float_to_string",
     hdrs = [
         "src/__support/float_to_string.h",
         "src/__support/ryu_constants.h",
     ],
     deps = [
         ":__support_cpp_type_traits",
         ":__support_fputil_fp_bits",
deps = [
         ":__support_common",
         ":__support_cpp_type_traits",
         ":__support_fputil_fp_bits",
         ":libc_root",
     ],
 )
 
 libc_support_library(
+    name = "__support_file_file",
+    hdrs = ["src/__support/File/file.h"],
+    srcs = ["src/__support/File/file.cpp"],
+    deps = [
+        ":libc_root",
+        ":__support_cpp_new",
+        ":__support_error_or",
+        ":__support_cpp_span",
+        ":__support_threads_mutex",
+        ":errno",
+    ],
+)
+
+libc_support_library(
     name = "__support_named_pair",
     hdrs = ["src/__support/named_pair.h"],
     deps = [":libc_root"],
 )
 
 libc_support_library(
     name = "__support_builtin_wrappers",
     hdrs = ["src/__support/builtin_wrappers.h"],
textual_hdrs = [
         #TODO: add support for GPU quick_exit (isn't just in a header.)
     ],
     deps = [
         ":__support_osutil_syscall",
         ":libc_root",
     ],
 )
 
+libc_support_library(
+    name = "__support_threads_mutex",
+    hdrs = [
+        "src/__support/threads/mutex.h",
+        "src/__support/threads/mutex_common.h",
+    ],
+    textual_hdrs = [
+        "src/__support/threads/linux/mutex.h",
+        "src/__support/threads/linux/futex_word.h",
+    ],
+    deps = [
+        ":libc_root",
+        ":__support_cpp_atomic",
+        ":__support_osutil_syscall",
+    ],
+)
 
 ############################### errno targets ################################
 
 libc_function(
     name = "errno",
     srcs = ["src/errno/libc_errno.cpp"],
     hdrs = ["src/errno/libc_errno.h"],
     deps = [
deps = [
         ":__support_cpp_string_view",
         ":string_memory_utils",
         ":printf_core_structs",
         ":libc_root",
     ],
 )
 
 libc_support_library(
+    name = "printf_file_writer",
+    hdrs = ["src/stdio/printf_core/file_writer.h"],
+    deps = [
+        ":__support_cpp_string_view",
+        ":__support_file_file",
+        ":printf_core_structs",
+        ":libc_root",
+    ],
+    copts = ["-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE"],
+)
+
+libc_support_library(
     name = "printf_writer",
     hdrs = ["src/stdio/printf_core/writer.h"],
     srcs = ["src/stdio/printf_core/writer.cpp"],
     deps = [
         ":__support_cpp_string_view",
         ":libc_root",
     ],
 )
libc_function(
     deps = [
         ":__support_arg_list",
         ":printf_main",
         ":printf_string_writer",
         ":printf_writer",
         ":errno",
     ],
 )
+
+libc_support_library(
+    name = "vfprintf_internal",
+    hdrs = ["src/stdio/printf_core/vfprintf_internal.h"],
+    deps = [
+        ":__support_arg_list",
+        ":__support_file_file",
+        ":__support_macros_attributes",
+        ":printf_main",
+        ":printf_file_writer",
+        ":printf_writer",
+    ],
+)
+
+libc_function(
+    name = "printf",
+    srcs = ["src/stdio/printf.cpp"],
+    hdrs = ["src/stdio/printf.h"],
+    deps = [
+        ":__support_arg_list",
+        ":vfprintf_internal",
+        ":errno",
+    ],
+    copts = ["-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE"],
+)
+
+
+libc_function(
+    name = "fprintf",
+    srcs = ["src/stdio/fprintf.cpp"],
+    hdrs = ["src/stdio/fprintf.h"],
+    deps = [
+        ":__support_arg_list",
+        ":vfprintf_internal",
+        ":errno",
+    ],
+    copts = ["-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE"],
+)

utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl

 # This file is licensed 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
 
 """LLVM libc starlark rules for tests.
 
 libc functions are created though the libc_build_rules.bzl:libc_function.
 They come in two flavors:
  - the internal one that is scoped into the `__llvm_libc` namespace.
  - the libc one that is the regular C function.
 
 When performing tests we make sure to always use the internal version.
 """
 
 load("//libc:libc_build_rules.bzl", "INTERNAL_SUFFIX")
 
-def libc_test(name, srcs, libc_function_deps, deps = [], **kwargs):
+def libc_test(name, srcs, libc_function_deps, deps = [], output_file = "", **kwargs):
     """Add target for a libc test.
 
     Args:
       name: Test target name
       srcs: List of sources for the test.
       libc_function_deps: List of libc_function targets used by this test.
       deps: The list of other libraries to be linked in to the test target.
       **kwargs: Attributes relevant for a cc_test. For example, name, srcs.

utils/bazel/llvm-project-overlay/libc/test/src/stdio/BUILD.bazel

 # This file is licensed 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
 
 # Tests for LLVM libc stdio.h functions.
 
 load("//libc/test:libc_test_rules.bzl", "libc_test")
 
 package(default_visibility = ["//visibility:public"])
 
 licenses(["notice"])
 
+
+cc_library(
+    name = "testdata_path_util",
+    hdrs = ["testdata_path_util.h"],
+)
+
 libc_test(
     name = "printf_parser_test",
     srcs = ["printf_core/parser_test.cpp"],
     libc_function_deps = [
     ],
     deps = [
       "//libc:printf_parser",
       "//libc:printf_core_structs",

 
 libc_test(
     name = "snprintf_test",
     srcs = ["snprintf_test.cpp"],
     libc_function_deps = [
         "//libc:snprintf",
     ],
 )
+
+libc_test(
+    name = "printf_test",
+    srcs = ["printf_test.cpp"],
+    libc_function_deps = [
+        "//libc:printf",
+    ],
+)
+
+libc_test(
+    name = "fprintf_test",
+    srcs = ["fprintf_test.cpp"],
+    libc_function_deps = [
+        "//libc:fprintf",
+    ],
+    deps = [
+        ":testdata_path_util",
+    ],
+    copts = ["-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE"],
+)