[libc] add tests to WrapperGen

[michaelrj]

Summary

This adds an initial test that can serve as a basis for other tests on
wrappergen.

[abrachet]

I think a lit test might be more appropriate. I had started setting this up a while ago in D75427, it might not be worth setting up for what will be probably just one test though.

[michaelrj]

I'm not quite sure why a lit test would be better in this situation, does it have better support for testing complete binaries?

[sivachandra]

libc/test/utils/CMakeLists.txt

3

Should there be an additional nesting tools/WrapperGen?

libc/test/utils/WrapperGen/CMakeLists.txt

1

The implementation is currently wrappergen specific. So, may be make it generic and move it up one level higher under libc/test/utils/tools?

10

Add ARGS or something equivalent - See below.

29

Instead of listing --path, get the command line args from the arguments to add_libc_tool_unittest.

30

Give a generic name like, say --tool?

31

Same as for --path.

45

This should be listed under DEPENDS of add_libc_tool_unittest?

48

You can move lines 47 and 48 into add_libc_tool_unittest function.

Also, use fully qualified names: https://github.com/llvm/llvm-project/blob/master/libc/cmake/modules/LLVMLibCTargetNameUtils.cmake

libc/test/utils/WrapperGen/wrappergen_test.cpp

27

Instead of this, we should perhaps have an option called apifile for which we pass in a test API file. This test file need not do anything fancy. Something like:

include "spec/spec.td"
include "spec/std.td"

70

Also add a test for aliases.

105

Can you list the brittle points and add TODOs separately for them?

[michaelrj]

address most of the comments

[michaelrj]

submit comments

libc/test/utils/WrapperGen/CMakeLists.txt

45

no, if libc-wrappergen is part of the DEPENDS block it tries to link it in as part of the tests, which causes a bunch of errors, thus it's added separately.

48

I'm not quite sure what you mean by "use fully qualified names" since the file you linked appears to only support paths inside libc and LLVMSupport is from https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/CMakeLists.txt#L79 unless you meant a different name?

[sivachandra]

libc/test/utils/WrapperGen/CMakeLists.txt

45

So, IIUC, add_libc_tool_unittest expects an optional DEPENDS argument but does nothing with it. If DEPENDS is confusing, use TOOL_TARGET may be and then you can call add_dependencies to add this tool as a dependency.

48

What I mean is, the wrappergen_test target should have a fully qualified name like libc.test.utils.tools.WrapperGen.wrappergen_test.

libc/test/utils/tools/CMakeLists.txt

2 ↗

(On Diff #308491)

You shouldn't need this because: https://github.com/llvm/llvm-project/blob/master/libc/CMakeLists.txt#L97

38 ↗

(On Diff #308491)

Fix.

libc/test/utils/tools/WrapperGen/CMakeLists.txt

6 ↗

(On Diff #308491)

Why list an empty DEPENDS?

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp

27 ↗

(On Diff #308491)

We should make this test independent of the target OS/machine. That is why I suggested using a test API file. Did you run into any problems with that approach?

48 ↗

(On Diff #308491)

If you use the test API file approach, you will not need to construct the path to the API file. Just use the path as listed on the command line.

[abrachet]

In D92137#2423446, @michaelrj wrote:

I'm not quite sure why a lit test would be better in this situation, does it have better support for testing complete binaries?

Yes. Here's an example test with FileCheck https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/llvm-nm/invalid-input.test

// Currently it's just comparing the output of the program to an exact string, this means that even a small formatting change would break this test.

This is what FileCheck was designed for

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp

36–39 ↗

(On Diff #308491)

These should be constructed in SetUp. I'm not sure the behavior is defined if they are discarded in TearDown then later reused after TearDown

51–56 ↗

(On Diff #308491)

Maybe

llvm::ExitOnError errCheck("Temporary file failed to initialize for libc-wrappergen tests.");
errCheck(STDOutFile);
errCheck(STDErrFile);

83–90 ↗

(On Diff #308491)

std::string::append(const char *) will expect that the string it is null terminated. ::read can't do that.

https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/MemoryBuffer.h#L86
llvm::MemoryBuffer may be easier to use.

121 ↗

(On Diff #308491)

Not needed. Nor are the brackets on these 1 line while statements

[michaelrj]

add testapi.td so that the tests are no longer platform dependent and clean up the code a little.

[michaelrj]

after talking with Siva about it it looks like we're going to stick with this for the moment and not go with lit tests. The reason is that currently the wrappergen unit tests about as dependent on LLVM support as wrappergen itself, adding lit tests would make portability potentially harder.

libc/test/utils/tools/WrapperGen/CMakeLists.txt

6 ↗

(On Diff #308491)

it was a leftover from when I was trying adding libc-wrappergen as a full dependency.

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp

27 ↗

(On Diff #308491)

no problems, I just hadn't done it yet. It's now done and at test/utils/tools/WrapperGen/testapi.td

36–39 ↗

(On Diff #308491)

Since I'm using test fixtures a new one is defined for each TEST_F (see documentation copied below) so each file path is actually different (I even tested this and got the following for one run with both existing tests:
wrappergen-stdout-5b-5b-2e-bb.txt
wrappergen-stderr-e2-c7-cb-19.txt
wrappergen-stdout-19-07-f4-2b.txt
wrappergen-stderr-db-35-0f-06.txt).
The reason I'm not initializing them in SetUp is because if I try to then when the class is instantiated it tries to give them a default value using the default constructor, but there is no default constructor for Expected<TempFile> so it just gives me an error.

For each test defined with TEST_F(), googletest will create a fresh test fixture at runtime, immediately initialize it via SetUp(), run the test, clean up by calling TearDown(), and then delete the test fixture. Note that different tests in the same test suite have different test fixture objects, and googletest always deletes a test fixture before it creates the next one. googletest does not reuse the same test fixture for multiple tests. Any changes one test makes to the fixture do not affect other tests.

48 ↗

(On Diff #308491)

I'm not sure what you mean by "use the path as listed on the command line", do you mean create a new command line argument for the api to use? Right now I'm still constructing the path, just in a more simple way.

51–56 ↗

(On Diff #308491)

I tried the code you provided but it gave me the following error:
not viable: no known conversion from 'llvm::Expected<llvm::sys::fs::TempFile>' to 'Expected<llvm::sys::fs::TempFile> &&' for 1st argument
I'll look into it more later but that's why the code is commented out for the moment.

83–90 ↗

(On Diff #308491)

yeah that makes this code a lot cleaner. Thank you

[michaelrj]

remove references to targetos

[sivachandra]

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp

36–39 ↗

(On Diff #308491)

In a later pass, we should move constructs like this into a tool-test-matcher which will make this look a lot more cleaner.

48 ↗

(On Diff #308491)

Yes, instead of the command line argument which is expecting a path to the libc directory, I am suggesting to use an argument which takes a path to the API file.

[michaelrj]

added more tests

[michaelrj]

submit comments

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp

48 ↗

(On Diff #308491)

I do still need the path to the libc directory since that's an argument for wrappergen, but I have added a new argument for the api file.

[sivachandra]

I think we can chisel this out to be a little more cleaner but we can do it in a later pass.

libc/test/utils/tools/WrapperGen/testapi.td

3 ↗

(On Diff #309077)

Fix.

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp

251 ↗

(On Diff #309077)

Fix.

[michaelrj]

final cleanup

libc/test/utils/CMakeLists.txt

 add_subdirectory(FPUtil)
 add_subdirectory(CPP)
+add_subdirectory(WrapperGen)

libc/test/utils/WrapperGen/CMakeLists.txt

+function(add_libc_tool_unittest target_name)
+  if(NOT LLVM_INCLUDE_TESTS)
+    return()
+  endif()
+
+  cmake_parse_arguments(
+    "LIBC_TOOL_UNITTEST"
+    "" # No optional arguments
+    "" # Single value arguments
+    "SRCS;DEPENDS" # Multi-value arguments
+    ${ARGN}
+  )
+
+  add_executable(${target_name}
+    EXCLUDE_FROM_ALL
+    ${LIBC_TOOL_UNITTEST_SRCS}
+  )
+  target_link_libraries(${target_name}
+    PRIVATE
+    gtest_main
+    gtest
+    ${LIBC_TOOL_UNITTEST_DEPENDS}
+  )
+
+  add_custom_command(
+    TARGET ${target_name}
+    POST_BUILD
+    COMMAND $<TARGET_FILE:${target_name}> 
+            --path=${LIBC_SOURCE_DIR}
+            --wrappergen=${CMAKE_BINARY_DIR}/bin/libc-wrappergen
+            --targetos=${LIBC_TARGET_OS}
+  )
+  add_dependencies(libc-tool-util-tests ${target_name})
+endfunction()
+
+
+add_custom_target(libc-tool-util-tests)
+
+add_libc_tool_unittest( wrappergen_test
+  SRCS 
+    wrappergen_test.cpp   
+  DEPENDS
+)
+
+add_dependencies(wrappergen_test libc-wrappergen)
+
+target_compile_options(wrappergen_test PUBLIC -fno-rtti)
+target_link_libraries(wrappergen_test PRIVATE LLVMSupport)
 No newline at end of file

libc/test/utils/WrapperGen/wrappergen_test.cpp

+//===-- Unittests for WrapperGen ------------------------------------------===//
+//
+// 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 "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Program.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <unistd.h>
+
+llvm::cl::opt<std::string>
+    LibcPath("path", llvm::cl::desc("Path to the top level libc directory."),
+             llvm::cl::value_desc("<path to libc>"), llvm::cl::Required);
+llvm::cl::opt<std::string> WrapperGenPath(
+    "wrappergen", llvm::cl::desc("Path to the wrappergen executable."),
+    llvm::cl::value_desc("<path to wrappergen>"), llvm::cl::Required);
+llvm::cl::opt<std::string>
+    TargetOS("targetos", llvm::cl::desc("Name of the target operating system."),
+             llvm::cl::value_desc("<name of target OS>"), llvm::cl::Required);
+
+class WrapperGenTest : public testing::Test {
+public:
+  std::string IncludeArg;
+  std::string APIArg;
+  llvm::StringRef ProgPath;
+  llvm::Expected<llvm::sys::fs::TempFile> STDOutFile =
+      llvm::sys::fs::TempFile::create("wrappergen-stdout-%%-%%-%%-%%.txt");
+  llvm::Expected<llvm::sys::fs::TempFile> STDErrFile =
+      llvm::sys::fs::TempFile::create("wrappergen-stderr-%%-%%-%%-%%.txt");
+
+protected:
+  void SetUp() override {
+    IncludeArg = "-I=";
+    IncludeArg.append(LibcPath);
+    APIArg = LibcPath;
+    APIArg.append("/config/");
+    APIArg.append(TargetOS);
+    APIArg.append("/api.td");
+    ProgPath = llvm::StringRef(WrapperGenPath);
+
+    if (!STDOutFile) {
+      llvm::errs() << "Error: " << llvm::toString(STDOutFile.takeError())
+                   << "\n";
+      llvm::report_fatal_error(
+          "Temporary file failed to initialize for libc-wrappergen tests.");
+    }
+    if (!STDErrFile) {
+      llvm::errs() << "Error: " << llvm::toString(STDErrFile.takeError())
+                   << "\n";
+      llvm::report_fatal_error(
+          "Temporary file failed to initialize for libc-wrappergen tests.");
+    }
+  }
+  void TearDown() override {
+    llvm::consumeError(STDOutFile.get().discard());
+    llvm::consumeError(STDErrFile.get().discard());
+  }
+};
+
+TEST_F(WrapperGenTest, RunWrapperGen) {
+  llvm::Optional<llvm::StringRef> Redirects[] = {
+      llvm::None, llvm::StringRef(STDOutFile.get().TmpName),
+      llvm::StringRef(STDErrFile.get().TmpName)};
+
+  llvm::StringRef ArgV[] = {ProgPath, llvm::StringRef(IncludeArg),
+                            llvm::StringRef(APIArg), "--name", "strlen"};
+
+  int ExitCode =
+      llvm::sys::ExecuteAndWait(ProgPath, ArgV, llvm::None, Redirects);
+
+  EXPECT_EQ(ExitCode, 0);
+
+  constexpr size_t BufferSize = 128;
+  char Buffer[BufferSize];
+
+  int STDErrFD = STDErrFile.get().FD;
+  std::string STDErrOutput;
+  while (read(STDErrFD, Buffer, BufferSize)) {
+    STDErrOutput.append(Buffer);
+  }
+  ASSERT_EQ(STDErrOutput, "");
+
+  int STDOutFD = STDOutFile.get().FD;
+  std::string STDOutOutput;
+
+  while (read(STDOutFD, Buffer, BufferSize)) {
+
+    STDOutOutput.append(Buffer);
+  }
+
+  ASSERT_EQ(STDOutOutput, "#include \"src/string/strlen.h\"\n"
+                          "extern \"C\" size_t strlen(const char * __arg0) {\n"
+                          "  return __llvm_libc::strlen(__arg0);\n"
+                          "}\n");
+  // TODO:(michaelrj) Figure out how to make this less brittle.
+}