Page MenuHomePhabricator

[LLDB] [test] Use %clang_cl instead of build.py in a few tests
ClosedPublic

Authored by mstorsjo on Oct 16 2019, 5:20 AM.

Details

Summary

This allows explicitly specifying the intended target architecture, for tests that aren't supposed to be executed, and that don't require MSVC headers or libraries to be available.

(These tests already implicitly assumed to be built for x86; one didn't specify anything, assuming x86_64, while the other specified --arch=32, which only picks the 32 bit variant of the default target architecture).

Join two comment lines in disassembly.cpp, to keep row numbers checked in the test unchanged.

This fixes running check-lldb on arm linux.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 16 2019, 5:20 AM

It would be super-awesome if this worked, as this is exactly how the DWARF moral equivalents of these tests (e.g. test/Shell/SymbolFile/DWARF/debug-types.test) work. Let's see what Stella says about this.

lldb/test/Shell/SymbolFile/NativePDB/disassembly.cpp
6

Why is lld-link not specified as %lld_link?

mstorsjo marked an inline comment as done.Oct 16 2019, 11:35 AM
mstorsjo added inline comments.
lldb/test/Shell/SymbolFile/NativePDB/disassembly.cpp
6

Because that's how the lit substitutions are set up currently, and that's how existing tests write it. The substitutions are defined here:
https://github.com/llvm/llvm-project/blob/master/llvm/utils/lit/lit/llvm/config.py#L410
https://github.com/llvm/llvm-project/blob/master/llvm/utils/lit/lit/llvm/config.py#L492

Not sure if it's just because these have randomly happened to diverge, or if the percent for clang indicates that it's not just replaced with a resolved path, but also might include a few preset options.

labath added inline comments.Oct 16 2019, 12:16 PM
lldb/test/Shell/SymbolFile/NativePDB/disassembly.cpp
6

Not sure if it's just because these have randomly happened to diverge, or if the percent for clang indicates that it's not just replaced with a resolved path, but also might include a few preset options.

I think that might have been the intention at some point, but I wouldn't count on it being applied consistently.

stella.stamenova accepted this revision.Oct 17 2019, 9:29 AM

The tests passed in my setup. After you commit this, please monitor the windows Buildbot (it currently has a couple of failures, so you will have to check it and not rely on an email).

lldb/test/Shell/SymbolFile/NativePDB/disassembly.cpp
6

If I recall correctly, this was one of the problems that build.py was trying to fix - because lld-link is not %lld_link sometimes it just called lld-link. It seems to be working though, so maybe we can use it as-is.

This revision is now accepted and ready to land.Oct 17 2019, 9:29 AM
This revision was automatically updated to reflect the committed changes.

Either this or https://reviews.llvm.org/D69076 broke the lldb-cmake bot: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/2706/

Script:
--
: 'RUN: at line 4';   /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang --driver-mode=cl -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/lldb-test-build.noindex/module-cache-clang/lldb-shell --target=i386-windows-msvc -Od -Z7 -c /Fo/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/SymbolFile/NativePDB/Output/function-types-calling-conv.cpp.tmp.obj -- /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/Shell/SymbolFile/NativePDB/function-types-calling-conv.cpp
: 'RUN: at line 5';   /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/lld-link -debug:full -nodefaultlib -entry:main /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/SymbolFile/NativePDB/Output/function-types-calling-conv.cpp.tmp.obj -out:/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/SymbolFile/NativePDB/Output/function-types-calling-conv.cpp.tmp.exe -pdb:/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/SymbolFile/NativePDB/Output/function-types-calling-conv.cpp.tmp.pdb
: 'RUN: at line 6';   env LLDB_USE_NATIVE_PDB_READER=1 /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/lldb --no-lldbinit -S /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/Shell/lit-lldb-init -f /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/SymbolFile/NativePDB/Output/function-types-calling-conv.cpp.tmp.exe -s      /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/Shell/SymbolFile/NativePDB/Inputs/function-types-calling-conv.lldbinit | /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/FileCheck /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/Shell/SymbolFile/NativePDB/function-types-calling-conv.cpp
--
Exit Code: 1

Command Output (stderr):
--
clang-10: warning: unknown argument ignored in clang-cl: '-isysroot' [-Wunknown-argument]
clang-10: warning: unknown argument ignored in clang-cl: '-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/lldb-test-build.noindex/module-cache-clang/lldb-shell' [-Wunknown-argument]
clang-10: error: cannot specify '/Fo/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/tools/lldb/test/SymbolFile/NativePDB/Output/function-types-calling-conv.cpp.tmp.obj' when compiling multiple source files
clang-10: warning: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk: 'linker' input unused [-Wunused-command-line-argument]

--

Probably this commit. Can you please revert or fix?

Probably this commit. Can you please revert or fix?

Sorry about this, reverted it for now.

I think the reason might be that %clang_cl ends up expanding to something (maybe -isysroot) that clang-cl doesn't parse properly but ends up parsing as an input filename, will try to reproduce it locally on macOS.

The tests passed in my setup. After you commit this, please monitor the windows Buildbot (it currently has a couple of failures, so you will have to check it and not rely on an email).

I reverted this due to issues when run on macOS, but it did seem to pass without adding any new failures to the windows bot, fwiw.

Check out lldb/test/Shell/helper/toolchain.py, you probably need to filter out some options for clang_cl specifically.

Check out lldb/test/Shell/helper/toolchain.py, you probably need to filter out some options for clang_cl specifically.

Yeah, I later found that. The issue is that it's passed to usr_clang in i llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.

Maybe the additional_flags param needs to be split, into one common for all, one for gcc-style driver, one for clang_cl, and maybe also a separate one for cc1 (where not all driver level flags may fit either)?

Check out lldb/test/Shell/helper/toolchain.py, you probably need to filter out some options for clang_cl specifically.

Yeah, I later found that. The issue is that it's passed to usr_clang in i llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.

Maybe the additional_flags param needs to be split, into one common for all, one for gcc-style driver, one for clang_cl, and maybe also a separate one for cc1 (where not all driver level flags may fit either)?

Actually, it seems to be that these arguments should not be added to the command line by default. All of the existing tests that do "%clang -target whatever" don't need the arguments, and they "only" work because the arguments do no harm because the tests don't do anything which would depend on them. I think we should leave additional_flags argument mostly empty, and instead have a separate way of invoking clang when building for the host. Either %clang_host (achievable with a recursive substitution %clang_host -> %clang <whatever>), or using something like %clang %host_cflags.

I can have a stab at that, if you like, but I'm not sure I'll get around to that before the llvm conference is over...

Check out lldb/test/Shell/helper/toolchain.py, you probably need to filter out some options for clang_cl specifically.

Yeah, I later found that. The issue is that it's passed to usr_clang in i llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.

Maybe the additional_flags param needs to be split, into one common for all, one for gcc-style driver, one for clang_cl, and maybe also a separate one for cc1 (where not all driver level flags may fit either)?

Actually, it seems to be that these arguments should not be added to the command line by default. All of the existing tests that do "%clang -target whatever" don't need the arguments, and they "only" work because the arguments do no harm because the tests don't do anything which would depend on them. I think we should leave additional_flags argument mostly empty, and instead have a separate way of invoking clang when building for the host. Either %clang_host (achievable with a recursive substitution %clang_host -> %clang <whatever>), or using something like %clang %host_cflags.

I can have a stab at that, if you like, but I'm not sure I'll get around to that before the llvm conference is over...

Ok, that makes sense.

I'd appreciate if you do that. I have no hurry wrt this patch as it's just a collateral fix I picked up along the way :-)

Check out lldb/test/Shell/helper/toolchain.py, you probably need to filter out some options for clang_cl specifically.

Yeah, I later found that. The issue is that it's passed to usr_clang in i llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.

Maybe the additional_flags param needs to be split, into one common for all, one for gcc-style driver, one for clang_cl, and maybe also a separate one for cc1 (where not all driver level flags may fit either)?

Actually, it seems to be that these arguments should not be added to the command line by default. All of the existing tests that do "%clang -target whatever" don't need the arguments, and they "only" work because the arguments do no harm because the tests don't do anything which would depend on them. I think we should leave additional_flags argument mostly empty, and instead have a separate way of invoking clang when building for the host. Either %clang_host (achievable with a recursive substitution %clang_host -> %clang <whatever>), or using something like %clang %host_cflags.

I can have a stab at that, if you like, but I'm not sure I'll get around to that before the llvm conference is over...

Do you have time to look into this now?

I'm afraid I haven't been able to write a single line of code since I got back from the devmtg, but I have this on my radar, and I fully intend to get back to it. :(

BTW, as I started working on this patch, I realized that there is a (somewhat surprising) workaround for this problem. If we change the isysroot argument to be passed as -isysroot/foo (instead of -isysroot /foo), then clang-cl will properly ignore this argument (as it already knows how to ignore most arguments). The main surprising thing here is that the argument is -isysroot/foo and not --isysroot=/foo, but maybe that could also be changed for the sake of consistency with --sysroot...

Ok, https://reviews.llvm.org/D69619 should resolve the issues you encountered, but I'd recommend waiting a while before building on top of that, because it has the potential to cause problems for some configurations. Also stella's bot is still down due to the github stuff, so we won't get windows signal here (though I guess we've already established that this patch works on windows in the previous attempt).

Ok, https://reviews.llvm.org/D69619 should resolve the issues you encountered, but I'd recommend waiting a while before building on top of that, because it has the potential to cause problems for some configurations. Also stella's bot is still down due to the github stuff, so we won't get windows signal here (though I guess we've already established that this patch works on windows in the previous attempt).

Great, thanks! I'll hold off for a bit to let the dust settle, and then try pushing this one e.g. tomorrow.

For the record, this was reapplied in rG1739c7c10c42748c278b0ea194e32bbfdd04fb98 (which Phabricator doesn't seem to have picked up on here) .