Page MenuHomePhabricator

[flang] Google test infrastructure support for unittests
ClosedPublic

Authored by sameeranjoshi on May 21 2020, 7:13 AM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald Transcript
This comment was removed by sameeranjoshi.

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

sameeranjoshi edited the summary of this revision. (Show Details)

Builds and tests work fine.
README updated.

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
200

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

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.

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/

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?

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

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

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

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?

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

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.

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
184–186
  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?
This revision is now accepted and ready to land.May 24 2020, 4:11 PM
flang/CMakeLists.txt
184–186

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.

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
31

Why is LLVM_SRC_ROOT needed for the tests?

34

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

schweitz added inline comments.May 26 2020, 8:40 AM
flang/test/Unit/lit.cfg.py
34

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 added inline comments.May 26 2020, 8:45 AM
flang/CMakeLists.txt
184–186

Agreed. This should not be changed.

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.

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 added a comment.EditedJun 2 2020, 9:28 AM

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.

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 added inline comments.Jun 2 2020, 11:28 AM
flang/CMakeLists.txt
137

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

138

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 marked 2 inline comments as done.Jun 3 2020, 1:30 AM
sameeranjoshi added inline comments.
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 marked 10 inline comments as done.Jun 3 2020, 5:23 AM
sameeranjoshi added inline comments.
flang/CMakeLists.txt
137

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
34

Removed them as they are not used currently.

tskeith requested changes to this revision.Jun 4 2020, 11:52 AM
tskeith added inline comments.
flang/CMakeLists.txt
137

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?

This revision now requires changes to proceed.Jun 4 2020, 11:52 AM
sameeranjoshi marked an inline comment as done.Jun 4 2020, 1:06 PM
sameeranjoshi added inline comments.
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 added inline comments.Jun 5 2020, 8:01 AM
flang/runtime/io-error.cpp
10 ↗(On Diff #267924)

You can answer this question with git log:

commit ea5efd1ea87e3d6b86a8e7f940f94dec0a90a004
Author: Isuru Fernando <isuruf@gmail.com>
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 marked 2 inline comments as done.
sameeranjoshi marked 5 inline comments as done.Jun 8 2020, 1:36 PM
sameeranjoshi added a subscriber: isuruf.
sameeranjoshi added inline comments.
flang/runtime/io-error.cpp
10 ↗(On Diff #267924)
sameeranjoshi marked an inline comment as done.Jun 9 2020, 11:36 AM

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

Meinersbur added inline comments.
flang/CMakeLists.txt
137

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 added inline comments.Jun 9 2020, 2:08 PM
flang/CMakeLists.txt
164

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

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

sameeranjoshi marked an inline comment as done.Jun 9 2020, 8:42 PM
sameeranjoshi added inline comments.
flang/CMakeLists.txt
137

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 added inline comments.Jun 10 2020, 6:41 AM
flang/CMakeLists.txt
137

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 edited the summary of this revision. (Show Details)
sameeranjoshi marked 4 inline comments as done.Jun 10 2020, 11:18 PM

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

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.

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 accepted this revision.Jun 11 2020, 4:37 PM

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

This revision is now accepted and ready to land.Jun 11 2020, 4:37 PM
DavidTruby added inline comments.Jun 12 2020, 9:09 AM
flang/CMakeLists.txt
160

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 accepted this revision.Jun 12 2020, 9:35 AM

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

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

sameeranjoshi marked 2 inline comments as done.Jun 12 2020, 10:14 AM

Formatting issue fixed.

isuruf added inline comments.Jun 12 2020, 3:34 PM
flang/CMakeLists.txt
132

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

sameeranjoshi marked 2 inline comments as done.Jun 12 2020, 10:43 PM
sameeranjoshi added inline comments.
flang/CMakeLists.txt
132

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

sameeranjoshi marked an inline comment as done.Jun 12 2020, 10:44 PM
isuruf added inline comments.Jun 12 2020, 10:51 PM
flang/CMakeLists.txt
132

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 marked an inline comment as done.Jun 13 2020, 8:49 AM
sameeranjoshi added inline comments.
flang/CMakeLists.txt
132

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

awarzynski added inline comments.Jun 15 2020, 5:53 AM
flang/CMakeLists.txt
132

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 closed this revision.Jun 15 2020, 9:46 AM

@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

@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 I have merged changes[1]

@sameeranjoshi Thank you, much appreciated!