Page MenuHomePhabricator

Refactor LLDB lit configuration files
ClosedPublic

Authored by zturner on Nov 1 2018, 3:42 PM.

Details

Summary

A year or so ago, I re-wrote most of the lit infrastructure in LLVM so that it wasn't so boilerplate-y. I added lots of common helper type stuff, simplifed usage patterns, and made the code more elegant and maintainable.

We migrated to this in LLVM, clang, and lld's lit files, but not in LLDBs. This started to bite me recently, as the 4 most recent times I tried to run the lit test suite in LLDB on a fresh checkout the first thing that would happen is that python would just start crashing with unhelpful backtraces and I would have to spend time investigating.

You can reproduce this today by doing a fresh cmake generation, doing ninja lldb and then python bin/llvm-lit.py -sv ~/lldb/lit/SymbolFile at which point you'll get a segfault that tells you nothing about what your problem is.

I started trying to fix the issues with bandaids, but it became clear that the proper solution was to just bring in the work I did in the rest of the projects. The side benefit of this is that the lit configuration files become much cleaner and more understandable as a result.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

zturner created this revision.Nov 1 2018, 3:42 PM
stella.stamenova accepted this revision.Nov 1 2018, 4:49 PM

Looks good. The formatting in lit.cfg.py is a bit messy (indentations, especially), but as long as the tests pass, this is an improvement :). Thanks!

This revision is now accepted and ready to land.Nov 1 2018, 4:49 PM

Looks good. The formatting in lit.cfg.py is a bit messy (indentations, especially), but as long as the tests pass, this is an improvement :). Thanks!

Is there something like clang-format for Python? Happy to fix it if so. (I don't do a lot of Python so it's hard for me to eyeball it and figure out what's wrong, so I have to say it looks fine to me :))

This revision was automatically updated to reflect the committed changes.

Hi Zachary, looks like this broke GreenDragon: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12087/console

Can you have a look please?

Fix incoming, sorry about that.

Looks good. The formatting in lit.cfg.py is a bit messy (indentations, especially), but as long as the tests pass, this is an improvement :). Thanks!

Is there something like clang-format for Python? Happy to fix it if so. (I don't do a lot of Python so it's hard for me to eyeball it and figure out what's wrong, so I have to say it looks fine to me :))

There's no clang-format for Python that I know of. Since python cares about indentation, it's always a good idea to make sure that we use the same number of spaces for all of the 'if's and such as it will occasionally lead to hard to track down bugs. There are (in the final version) of lit.cfg.py a couple of places where two spaces make the indentation and everything else uses four. It's not a big deal because of where they are, but something to look out for.

Several of the windows tests that invoke clang-cl have started failing recently (I am not sure exactly when) and I suspect this change is the culprit. Were you able to run the tests successfully with this change?

I have not run the dotest suite recently, is that where you’re seeing the
failures? I successfully ran the lit suite and unit tests though

I have not run the dotest suite recently, is that where you’re seeing the
failures? I successfully ran the lit suite and unit tests though

Yes, they are primarily in the lldb-suite but not only:

http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1129/steps/test/logs/stdio

There are some other failures, but what I suspect happened after this change is that the tests are now not picking up clang-cl correctly:

$ "clang-cl" "/Zi" "E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\Expr/Inputs/call-function.cpp" "-o" "E:\build_slave\lldb-x64-windows-ninja\build\tools\lldb\lit\Expr\Output\TestIRMemoryMapWindows.test.tmp"
# command stderr:
clang-cl: warning: unable to find a Visual Studio installation; try running Clang from a developer command prompt [-Wmsvc-not-found]

clang-cl: error: unable to execute command: program not executable

clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)

I will have some time to have a look in more detail tomorrow.

It’s possible we lost some environment variable propagation, that would do
it. But I’m curious how it was finding the visual studio installation
before my patch.

It also looks like it’s failing finding link.exe (we really should make
lld-link the default). Another fix is to pass -fuse-ld=lld or split the
clang-cl line to separate compiler and linker invocations

I haven't verified this yet - but I suspect it is now picking up the clang-cl that comes with VS rather than the one that was just built.

Actually maybe it’s the other way around. Are you specifying
LLDB_TEST_COMPILER? If it’s picking up VS’s version, it would definitely be
able to find link.exe

The builds don't specify the two options for LLDB_TEST_C/CXX_COMPILER, so they should be picking up the freshly build compilers. We don't have an option for clang-cl though - so it's never been explicitly specified.

I also have some strange failures on Windows x86 (I run tests from x64_86 MSVC command line). If I locally revert this commit and 3 fix commits right after this one, then all seems to work. Here is the failure:

C:\Work\llvm\build_x86\bin>llvm-lit.py -v C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB\enums-layout.test
llvm-lit.py: C:/Work/llvm\utils\lit\lit\llvm\config.py:333: note: using C:/Work/llvm/build_x86/./bin/clang.exe: C:\Work\llvm\build_x86\bin\clang.exe
llvm-lit.py: C:/Work/llvm\utils\lit\lit\llvm\config.py:333: note: using C:/Work/llvm/build_x86/./bin/clang++.exe: C:\Work\llvm\build_x86\bin\clang++.exe
config.cc = C:\Work\llvm\build_x86\bin\clang.exe
-- Testing: 1 tests, 1 threads --
FAIL: LLDB :: SymbolFile/PDB/enums-layout.test (1 of 1)
******************** TEST 'LLDB :: SymbolFile/PDB/enums-layout.test' FAILED ********************
Script:
--
: 'RUN: at line 2';   clang-cl -m32 /Z7 /c /GS- C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp /o C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj
: 'RUN: at line 3';   link C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj /DEBUG /nodefaultlib /ENTRY:main /OUT:C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.exe
: 'RUN: at line 4';   C:\Work\llvm\build_x86\bin\lldb-test.EXE symbols C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.exe | C:\Work\llvm\build_x86\bin\FileCheck.EXE C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB\enums-layout.test
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 2"
$ "clang-cl" "-m32" "/Z7" "/c" "/GS-" "C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp" "/o" "C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj"
# command stderr:
C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp(41,9) :  error: unknown type name 'char16_t'
typedef char16_t WChar16Typedef;
        ^
C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp(44,9) :  error: unknown type name 'char32_t'
typedef char32_t WChar32Typedef;
        ^
2 errors generated.

error: command failed with exit status: 1

--

********************
Testing Time: 0.33s
********************
Failing Tests (1):
    LLDB :: SymbolFile/PDB/enums-layout.test

  Unexpected Failures: 1

I can't figure out yet what can be wrong with it. May be you have some ideas how to fix this?

But all compiles without errors if I run this manually:

clang-cl -m32 /Z7 /c /GS- C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp /o C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj

I think it must be related to setting up the environment in which to run
clang. In all other projects we call llvm_config.use_clang() which is in
llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path
of a clang we are trying to use, we skip this function in LLDB's lit
configuration files. But there is also a lot of other logic in that
function, so perhaps it's some of that logic that's necessary.

I took a brief look and I have a question about the usage of clang (rather than clang-cl).

In general I would agree that we have an exact path of clang (or gcc) that we are trying to use and they’re specified by using %cc and %cxx in the test files, but there are a number of test files that simply use clang e.g.:

SymbolFile\DWARF\find-variable-dwo.cpp:3:// RUN: clang %s -g -gsplit-dwarf -c -emit-llvm -o - --target=x86_64-pc-linux -DONE

In this case, are we not going to pick up whatever clang happens to be in the path instead of one that was explicitly specified? Is this intentional?

Thanks,
-Stella

From: Zachary Turner <zturner@google.com>
Sent: Tuesday, November 13, 2018 2:46 PM
To: reviews+D54009+public+0e164460da8f1d7f@reviews.llvm.org
Cc: Stella Stamenova <stilis@microsoft.com>; pavel@labath.sk; chris.bieneman@me.com; dccitaliano@gmail.com; aleksandr.urakov@jetbrains.com; jdevlieghere@apple.com; abidh.haq@gmail.com; teemperor@gmail.com; ki.stfu@gmail.com; mgorny@gentoo.org; dan@su-root.co.uk; jfbastien@apple.com; lldb-commits@lists.llvm.org; llvm@inglorion.net
Subject: Re: [PATCH] D54009: Refactor LLDB lit configuration files

I think it must be related to setting up the environment in which to run clang. In all other projects we call llvm_config.use_clang() which is in llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path of a clang we are trying to use, we skip this function in LLDB's lit configuration files. But there is also a lot of other logic in that function, so perhaps it's some of that logic that's necessary.