[flang] Google test infrastructure support for unittests

[sameeranjoshi]

Summary

Adds google test infrastructure support for in tree and out-of-tree.
Updates README.
Adds new target `check-flang-unit`

[sameeranjoshi]

This comment was removed by sameeranjoshi.

[tskeith]

Would you pleave update the build and test instructions in flang/README.md?

[sameeranjoshi]

Builds and tests work fine.
README updated.

[tskeith]

When I try to build I get this error:

ld: library not found for -lgtest_main

Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.

flang/README.md

168 ↗

(On Diff #265675)

How do you run unit tests for out-of-tree builds?

[sameeranjoshi]

In D80377#2051051, @tskeith wrote:

When I try to build I get this error:

ld: library not found for -lgtest_main

From a cursory look #include <gtest/gtest.h>should have been #include "gtest/gtest.h". As I had gtest package installed.

[sameeranjoshi]

In D80377#2051051, @tskeith wrote:

When I try to build I get this error:

ld: library not found for -lgtest_main

Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.

I tried twice building the compiler in-tree on 2 different m/c. I couldn't reproduce the issue, here's how my cmake looks like:

cmake -G "Ninja"  -DCMAKE_INSTALL_PREFIX=../install/  -DCMAKE_BUILD_TYPE="Release"  -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS="mlir;flang" -DCMAKE_CXX_STANDARD=17 -DFLANG_INCLUDE_TESTS=ON ../llvm-project/llvm/

[tskeith]

In D80377#2051422, @sameeranjoshi wrote:

In D80377#2051051, @tskeith wrote:

When I try to build I get this error:

ld: library not found for -lgtest_main

Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.

I tried twice building the compiler in-tree on 2 different m/c. I couldn't reproduce the issue, here's how my cmake looks like:

cmake -G "Ninja"  -DCMAKE_INSTALL_PREFIX=../install/  -DCMAKE_BUILD_TYPE="Release"  -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS="mlir;flang" -DCMAKE_CXX_STANDARD=17 -DFLANG_INCLUDE_TESTS=ON ../llvm-project/llvm/

Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?

[sameeranjoshi]

In D80377#2052113, @tskeith wrote:

In D80377#2051422, @sameeranjoshi wrote:

In D80377#2051051, @tskeith wrote:

When I try to build I get this error:

ld: library not found for -lgtest_main

Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.

I tried twice building the compiler in-tree on 2 different m/c. I couldn't reproduce the issue, here's how my cmake looks like:

cmake -G "Ninja"  -DCMAKE_INSTALL_PREFIX=../install/  -DCMAKE_BUILD_TYPE="Release"  -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS="mlir;flang" -DCMAKE_CXX_STANDARD=17 -DFLANG_INCLUDE_TESTS=ON ../llvm-project/llvm/

Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?

Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest

[sameeranjoshi]

In D80377#2052113, @tskeith wrote:

In D80377#2051422, @sameeranjoshi wrote:

In D80377#2051051, @tskeith wrote:

When I try to build I get this error:

ld: library not found for -lgtest_main

Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.

I tried twice building the compiler in-tree on 2 different m/c. I couldn't reproduce the issue, here's how my cmake looks like:

cmake -G "Ninja"  -DCMAKE_INSTALL_PREFIX=../install/  -DCMAKE_BUILD_TYPE="Release"  -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS="mlir;flang" -DCMAKE_CXX_STANDARD=17 -DFLANG_INCLUDE_TESTS=ON ../llvm-project/llvm/

Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?

Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest

In D80377#2052155, @sameeranjoshi wrote:

In D80377#2052113, @tskeith wrote:

In D80377#2051422, @sameeranjoshi wrote:

In D80377#2051051, @tskeith wrote:

When I try to build I get this error:

ld: library not found for -lgtest_main

Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.

I tried twice building the compiler in-tree on 2 different m/c. I couldn't reproduce the issue, here's how my cmake looks like:

cmake -G "Ninja"  -DCMAKE_INSTALL_PREFIX=../install/  -DCMAKE_BUILD_TYPE="Release"  -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS="mlir;flang" -DCMAKE_CXX_STANDARD=17 -DFLANG_INCLUDE_TESTS=ON ../llvm-project/llvm/

Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?

Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest

Also you should try considering below commit in fir-dev.
https://github.com/flang-compiler/f18-llvm-project/pull/91

This would add the check-flang-unit target, without the FIR dependency, so the other pending post merge task
https://github.com/flang-compiler/f18/issues/995
could not have a blocker.

And the out-of-tree building would still fail as there are issues in building flang with out of tree with FLANG_INCLUDE_TESTS=ON, that's the reason I didn't mention in README

[tskeith]

Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?

Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest

Where is gtest_main found in your build?

[sameeranjoshi]

In D80377#2052243, @tskeith wrote:

Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?

Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest

Where is gtest_main found in your build?

It's in build/lib/libgtest_main.a

[tskeith]

Where is gtest_main found in your build?

It's in build/lib/libgtest_main.a

Thanks, I found the problem: I had LLVM_INCLUDE_TESTS set to Off and apparently it must be On for gtest to get built.
So the cmake files should either force it to be on when FLANG_INCLUDE_TESTS is on, or report a fatal error when they are set inconsistently.

[kiranchandramohan]

Thanks @sameeranjoshi for working on this.

check-flang-unit works for me with ninja on Ubuntu Linux on AArch64 in-tree build.

Please wait for @tskeith to approve.

flang/CMakeLists.txt

132 ↗

(On Diff #265675)

1. Does this mean that the flang unit tests cannot be controlled using LLVM_INCLUDE_TESTS?
2. Do you know why Clang/LLVM has unittests set to OFF by default?

[richard.barton.arm]

flang/CMakeLists.txt

132 ↗

(On Diff #265675)

I don't think this is right. LLVM_INCLUDE_TESTS is On by default according to llvm/docs/CMake.rst. LLVM so we should be able to keep the original code. This is what the other LLVM sub-projects seem to do, i.e. give you a local toggle for the unit tests of the sub-project, but default to the global setting for unit tests.

Note there is LLVM_BUILD_TESTS, which defaults to OFF, that excludes the tests from the all target. This means the unittests will be built only on make check-all rather than on make.

[richard.barton.arm]

I think the code to handle the issue that @tskeith points out in clang standalone builds is here https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L187. The key is to add a dependency on the utils/unittest directory.

There is also a GTest format that should be set in the lit.cfg https://github.com/llvm/llvm-project/blob/master/clang/test/Unit/lit.cfg.py#L24.

flang/test/Unit/lit.cfg.py

30 ↗

(On Diff #265675)

Why is LLVM_SRC_ROOT needed for the tests?

33 ↗

(On Diff #265675)

Other sub-projects set this up in top-level CMakeLists using FindPythonInterp. Why do we set it here?

[schweitz]

flang/test/Unit/lit.cfg.py

33 ↗

(On Diff #265675)

Sameeran was pulling this glue code from the lld subproject, where these config settings are made. They can probably be deleted here. I do not appear to be used, but I haven't tested it.

[schweitz]

flang/CMakeLists.txt

132 ↗

(On Diff #265675)

Agreed. This should not be changed.

[sameeranjoshi]

All the changes were made for handling out-of-tree errors, no changes in in-tree handling code.

1. Some changes were ported from fir-dev to resolve out-of-tree errors and some API changes which were bugs and merged in fir-dev .
2. A sample gtest is added.
3. Changes in Cmake to support out-of-tree builds of unittests
4. README updated for out-of-tree setup

note: check-flang-unit won't work for out-of-tree till D81002 is merged.

[tskeith]

It seems to me there is something wrong with this patch. The paths are relative to flang/ rather that the root of the repo so the patch doesn't apply successfully. Or maybe there is something I'm missing about how phabricator works?

[DavidTruby]

In D80377#2068956, @tskeith wrote:

It seems to me there is something wrong with this patch. The paths are relative to flang/ rather that the root of the repo so the patch doesn't apply successfully. Or maybe there is something I'm missing about how phabricator works?

The root of the repo is llvm-project, of which flang is a subdirectory. The patch seems incorrect to me too on that basis.

[sameeranjoshi]

In D80377#2068956, @tskeith wrote:

It seems to me there is something wrong with this patch. The paths are relative to flang/ rather that the root of the repo so the patch doesn't apply successfully. Or maybe there is something I'm missing about how phabricator works?

Sorry I made a mistake. I was working out-of tree and that caused me to think in usual way when updating diff.
Please check this and thanks for pointing.

[tskeith]

flang/CMakeLists.txt

128 ↗

(On Diff #267924)

Why do you have to check for gtest.h? Won't it always be there?

129 ↗

(On Diff #267924)

Same question. Why do you have to check this?

flang/include/flang/Optimizer/Support/InternalNames.h

68 ↗

(On Diff #267924)

This doesn't look properly formatted. Is it part of your change?

flang/runtime/io-error.cpp

10 ↗

(On Diff #267924)

Is this supposed to be part of your change?

[sameeranjoshi]

flang/runtime/io-error.cpp

10 ↗

(On Diff #267924)

Yes, out-of-tree builds fail with this include.
Also see this commit addressing the same
https://github.com/flang-compiler/f18-llvm-project/commit/4069068c997812c1c8ef6434a5e630603c7232e0

[sameeranjoshi]

flang/CMakeLists.txt

128 ↗

(On Diff #267924)

Not always because

The out-of-tree build can be configured against the CMAKE_INSTALL_PREFIX directory of llvm, rather than the build directory.
LLVM does not install gtest when installing itself, meaning that gtest might not be available.

Also other subprojects as well follow something similar
[1]https://github.com/llvm/llvm-project/blob/8e058feae0b0d07cd86257f0aa3154acfa887fe0/clang/CMakeLists.txt#L188
[2]https://github.com/llvm/llvm-project/blob/82aac878beb48cd326b4684918b7ff2375fae439/polly/CMakeLists.txt#L29
[3]https://github.com/llvm/llvm-project/blob/82aac878beb48cd326b4684918b7ff2375fae439/lld/CMakeLists.txt#L109

flang/include/flang/Optimizer/Support/InternalNames.h

68 ↗

(On Diff #267924)

Yes it is.
I will format it.
This was a bug fixed in fir-dev[1] just porting it here.
https://github.com/flang-compiler/f18-llvm-project/pull/87/files

flang/test/Unit/lit.cfg.py

33 ↗

(On Diff #265675)

Removed them as they are not used currently.

[tskeith]

flang/CMakeLists.txt

128 ↗

(On Diff #267924)

If the user requests unit tests and we can't run them it should be an error, not silently ignored.

flang/include/flang/Optimizer/Support/InternalNames.h

68 ↗

(On Diff #267924)

This should be in a separate change. It has nothing to do with google test infrastructure.

flang/runtime/io-error.cpp

10 ↗

(On Diff #267924)

This should be in a separate change. It has nothing to do with google test infrastructure.

The include of config.h was added for a reason. Does that reason no longer apply or are you breaking something else by changing this?

[sameeranjoshi]

flang/runtime/io-error.cpp

10 ↗

(On Diff #267924)

I could build and test successfully with removing this file for out of tree and in tree.
Can you mention what does "something else" mean here or for what specific reason was this added ?

[tskeith]

flang/runtime/io-error.cpp

10 ↗

(On Diff #267924)

You can answer this question with git log:

commit ea5efd1ea87e3d6b86a8e7f940f94dec0a90a004
Author: Isuru Fernando <[e-mail]>
Date:   Thu Mar 12 15:28:35 2020 -0500

    [flang] Support platforms without strerror_r

    Original-commit: flang-compiler/f18@0575b54cc79110ae03f73b136e704f6313e2d63f
    Reviewed-on: https://github.com/flang-compiler/f18/pull/1068

[sameeranjoshi]

flang/runtime/io-error.cpp

10 ↗

(On Diff #267924)

Addressed by https://reviews.llvm.org/D81266
Thanks @isuruf

[sameeranjoshi]

@tskeith are these changes what you expect?
Is that ready to merge?

[Meinersbur]

flang/CMakeLists.txt

128 ↗

(On Diff #267924)

If the user is running make FlangUnitTests, yes. The build target may not exist in this case. But failing on make check-flang would mean the build cannot be verified at all if configuring against a pre-installed LLVM. If not supported, I would question the point of a flang standalone build.

Note that it is never possible to run all tests all the time since some tests have requirements, such as running on Windows. A warning about skipped tests might be useful though.

[tskeith]

flang/CMakeLists.txt

155 ↗

(On Diff #269340)

I get this. Is it what you're expecting?

CMake Error at CMakeLists.txt:164 (message):
  gtest.h is missing from /utils/unittest/googletest/include/gtest

flang/README.md

168 ↗

(On Diff #269340)

Somewhere in here you should explain that you can't run unit tests in an out-of-tree build against an LLVM install.

[sameeranjoshi]

flang/CMakeLists.txt

128 ↗

(On Diff #267924)

Agreed that a warning is useful.

What if we use CMake to download GoogleTest as part of the build's configure step[1] specifically for standalone builds? It definitely would increase configuration time, but we are assured the unit tests they run perfectly well.

[1]https://github.com/google/googletest/blob/master/googletest/README.md#incorporating-into-an-existing-cmake-project

[tskeith]

flang/CMakeLists.txt

128 ↗

(On Diff #267924)

I think it would make more sense to install GoogleTest as part of the LLVM install, as it currently done with testing tools like llvm-lit and FileCheck. Then this should work the same whether built against a build or install of LLVM. If you decide to do this it should be a separate change.

In the meantime, a meaningful warning is fine with me.

[sameeranjoshi]

@tskeith is that ready to merge?
I have addressed the comments.

[schweitz]

Have whatever changes made here been propagated to the fir-dev branch? I don't want to try to decipher which changes are needed, why things fail (if they do), etc.

[richard.barton.arm]

The status of the fir-dev branch has no bearing on this patch review.

If @sameeranjoshi wants to update fir-dev after pushing this change to master then that is up to him, but that should not in any way block committing the code to master.

[tskeith]

It would be good if someone who understand cmake reviews this.

[DavidTruby]

flang/CMakeLists.txt

160 ↗

(On Diff #270037)

I think it's better to use CMake's find_package(Threads) support here, so that this is more likely work on platforms without pthreads.
See: https://cmake.org/cmake/help/v3.8/module/FindThreads.html

[DavidTruby]

My above comment was just a small nit, I'm happy to approve this either way

[isuruf]

I'd like to see the -lpthread issue getting fixed before merging this.

[sameeranjoshi]

@isuruf done.

[sameeranjoshi]

Formatting issue fixed.

[isuruf]

flang/CMakeLists.txt

132 ↗

(On Diff #270448)

Is the variable LLVM_INCLUDE_TESTS set in a out-of-tree build?

[sameeranjoshi]

flang/CMakeLists.txt

132 ↗

(On Diff #270448)

For an out-of-tree build of flang the user is supposed to set FLANG_INCLUDE_TESTS if he wants a local toggle for the unit tests of sub project(flang in this case).

Now LLVM_INCLUDE_TESTS depends on how LLVM was built, but by default LLVM_INCLUDE_TESTS is ON.
In case, if LLVM_INCLUDE_TESTS was OFF still the user of out-of-tree build of flang can get unit-tests using the local toggle of FLANG_INCLUDE_TESTS.

Can read more on this on my older revision[1]
[1] https://reviews.llvm.org/D80377#2054070

[isuruf]

flang/CMakeLists.txt

132 ↗

(On Diff #270448)

LLVM_INCLUDE_TESTS is not set to any value in an out-of-tree build. It's only set if you are doing an in-tree build.

[sameeranjoshi]

flang/CMakeLists.txt

132 ↗

(On Diff #270448)

so @isuruf what should be the take on this?
Do you want me to set it to ON always for out-of-tree?

[awarzynski]

flang/CMakeLists.txt

132 ↗

(On Diff #270448)

This patch seems to be ready for in-tree testing, which is great! We are keen to start adding unit tests - could the discussion about out-of-tree testing be moved to a separate patch? Ta!

[sameeranjoshi]

@isuruf I have set FLANG_INCLUDE_TESTS to ON by default for out-of-tree builds.
If there are still any concerns I can fix in other PR.

@awarzynski I have merged changes[1]

[1] https://github.com/llvm/llvm-project/commit/93f602b339f62e058ce2c212c86f9f0ca3b20126

[kiranchandramohan]

@sameeranjoshi, the llvm commit process recommends that you add the Differential Revision message to your commit message. This will also automatically close the patch in review.
https://llvm.org/docs/Phabricator.html#committing-a-change

[awarzynski]

@awarzynski I have merged changes[1]

@sameeranjoshi Thank you, much appreciated!

flang/unittests/CMakeLists.txt

+add_custom_target(check-flang-unit)
+set_target_properties(check-flang-unit PROPERTIES FOLDER "Flang Unit Tests")
+
+function(add_flang_unittest test_dirname)
+  add_unittest(check-flang-unit ${test_dirname} ${ARGN})
+endfunction()
+
+add_subdirectory(Optimizer)
+
 add_subdirectory(Decimal)
 add_subdirectory(Evaluate)
 add_subdirectory(Runtime)

flang/unittests/Optimizer/CMakeLists.txt

+get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
+
+set(LIBS
+  FIRDialect
+  ${dialect_libs}
+)
+
+add_flang_unittest(FlangOptimizerTests
+  InternalNamesTest.cpp
+)
+target_link_libraries(FlangOptimizerTests
+  PRIVATE
+  ${LIBS})

flang/unittests/Optimizer/InternalNamesTest.cpp

+#include "flang/Optimizer/Support/InternalNames.h"
+#include <gtest/gtest.h>
+
+using namespace fir;
+using namespace llvm;
+
+TEST(genericName, MyTest) {
+        NameUniquer obj;
+        std::string val = obj.doCommonBlock("hello");
+  std::string val2 = "hello";
+  EXPECT_EQ(val, val2);
+}
+
+int main(int argc, char **argv) {
+  testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}
+