Don't set $LIB if we're not targetting windows.

[samsonov]

Timur, do we use MSVC to link/run ASan tests at all? If not, maybe we should just delete this piece of code?

[timurrrr]

Alexey,
Of course we do use MSVC link.exe to link ASan tests.
LLD is not ready for ASan on Windows as it doesn't produce debug info.

test/lit.common.cfg

64

I don't understand the comment and how it is related to the change in the code.

You've probably meant to write the condition meaning "if MSVC:" ?
If so, I don't think you need an extra comment:

# On Windows, help MSVS link.exe find the standard libraries.
if platform.system() == 'Windows' and ...:

Deferring the condition to Reid and Hans who know the triples better.

[filcab]

The comment came with my private code. But I don't feel strongly about it.

The thing is: we might be running on Windows, but targeting a different OS.
In which case we don't want to pass LIB along. Your toolchain runs on
Windows, but you're compiling for arch-vendor-anotheros.

Wouldn't lld on Windows also look at LIB if it was emulating link.exe (I
don't know if there's a mode for that, like for emulating ld)?

[timurrrr]

The comment came with my private code. But I don't feel strongly about it.

I see.

The thing is: we might be running on Windows, but targeting a different
OS. In which case we don't want to pass LIB along. Your toolchain runs on
Windows, but you're compiling for arch-vendor-anotheros.

I'm not sure how cross-compilation of tests works in LLVM.
Do we ever invoke LIT for anything that we can't run on the current system?

Wouldn't lld on Windows also look at LIB if it was emulating link.exe (I
don't know if there's a mode for that, like for emulating ld)?

Good point, I'm pretty sure LLD would need LIB to be set too.

[hans]

I'm not sure I understand what's being fixed here.

LLVM's test/lit.cfg seems to just set LIB unconditionally (test/lit.cfg:75).

A little higher up in this file we're conditionally putting INCLUDE in possibly_dangerous_env_vars, which seems to suggest that it that one also gets propagated through (but I can't see where that happens). Shouldn't we have the same logic for INCLUDE and LIB?

[filcab]

I may be misunderstanding, but:
It seems to me that, most likely, you don't want INCLUDE, since you want to
be sure of all the headers you include (and tests depend on a minimum set
of headers).

But you want LIB (on Windows targetting Windows) since that's where you can
find kernel32.dll and others.

LLVM tests only run on the host, so we don't have the “problem” of Windows
targetting some other platform. Because of that, we can simply forward LIB.

Filipe

[rnk]

I think this is probably a good change. I believe the compiler-rt test suite supports running the tests one some OS other than the host, and this supports that on Windows.

test/lit.common.cfg

64

For the triple check, how about:

"-win" not in config.target_triple

This will cover *-windows-msvc, *-windows, and *-win32. It will also cover *-windows-gnu, but that matches current behavior and it shouldn't matter because GNU ld doesn't respect LIB so far as I know.

[filcab]

Addressed Reid's comments

[timurrrr]

LGTM with a nit

test/lit.common.cfg

64–65

nit: please end the sentence with a period:
# Make sure we only try to use it when targetting Windows.

test/lit.common.cfg

 # Tweak PATH to include llvm tools dir.
 llvm_tools_dir = getattr(config, 'llvm_tools_dir', None)
 if (not llvm_tools_dir) or (not os.path.exists(llvm_tools_dir)):
   lit_config.fatal("Invalid llvm_tools_dir config attribute: %r" % llvm_tools_dir)
 path = os.path.pathsep.join((llvm_tools_dir, config.environment['PATH']))
 config.environment['PATH'] = path
 
 # Help MSVS link.exe find the standard libraries.
-if platform.system() == 'Windows':
+# Make sure we only try to use it when targetting Windows
+if platform.system() == 'Windows' and config.target_triple.endswith('win32'):
   config.environment['LIB'] = os.environ['LIB']
 
 # Use ugly construction to explicitly prohibit "clang", "clang++" etc.
 # in RUN lines.
 config.substitutions.append(
     (' clang', """\n\n*** Do not use 'clangXXX' in tests,
      instead define '%clangXXX' substitution in lit config. ***\n\n""") )