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

[mstorsjo]

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.

[labath]

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.

[stella.stamenova]

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

6

Why is lld-link not specified as %lld_link?

[mstorsjo]

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]

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]

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.

[aprantl]

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]

--

[aprantl]

Probably this commit. Can you please revert or fix?

[mstorsjo]

In D69031#1713532, @aprantl wrote:

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.

[mstorsjo]

In D69031#1713060, @stella.stamenova wrote:

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.

[aprantl]

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

[mstorsjo]

In D69031#1713900, @aprantl wrote:

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)?

[labath]

In D69031#1714107, @mstorsjo wrote:

In D69031#1713900, @aprantl wrote:

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...

[mstorsjo]

In D69031#1714143, @labath wrote:

In D69031#1714107, @mstorsjo wrote:

In D69031#1713900, @aprantl wrote:

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 :-)

[mstorsjo]

In D69031#1714143, @labath wrote:

In D69031#1714107, @mstorsjo wrote:

In D69031#1713900, @aprantl wrote:

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?

[labath]

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. :(

[labath]

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...

[labath]

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).

[mstorsjo]

In D69031#1728380, @labath wrote:

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.

[mstorsjo]

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

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

 // clang-format off
 // REQUIRES: lld
 
 // Test that we can show disassembly and source.
-// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s 
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -c /Fo%t.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
 // RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
 // RUN:     %p/Inputs/disassembly.lldbinit | FileCheck %s
 
-// Some context lines before
-// the function.
+// Some context lines before the function.
 
 int foo() { return 42; }
 
 int main(int argc, char **argv) {
   foo();
   return 0;
 }
 

lldb/test/Shell/SymbolFile/NativePDB/function-types-calling-conv.cpp

 // clang-format off
 // REQUIRES: lld
 
-// RUN: %build --compiler=clang-cl --arch=32 --nodefaultlib -o %t.exe -- %s 
+// RUN: %clang_cl --target=i386-windows-msvc -Od -Z7 -c /Fo%t.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
 // RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
 // RUN:     %p/Inputs/function-types-calling-conv.lldbinit | FileCheck %s
 
 
 void __stdcall StdcallFn() {}
 void __fastcall FastcallFn() {}
 void __thiscall ThiscallFn() {}
 void __cdecl CdeclFn() {}