Page MenuHomePhabricator

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

Authored by filcab on Feb 18 2015, 3:38 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

filcab updated this revision to Diff 20228.Feb 18 2015, 3:38 PM
filcab retitled this revision from to Don't set $LIB if we're not targetting windows..
filcab updated this object.
filcab edited the test plan for this revision. (Show Details)
filcab added reviewers: timurrrr, samsonov.
filcab added a subscriber: Unknown Object (MLST).
samsonov edited edge metadata.Feb 18 2015, 3:47 PM

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

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 ↗(On Diff #20228)

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.

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

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 edited edge metadata.Feb 19 2015, 9:52 AM

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?

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 edited edge metadata.Feb 19 2015, 12:58 PM

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 ↗(On Diff #20228)

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 updated this revision to Diff 20359.Feb 19 2015, 4:47 PM
filcab edited edge metadata.

Addressed Reid's comments

timurrrr accepted this revision.Feb 20 2015, 7:51 AM
timurrrr edited edge metadata.

LGTM with a nit

test/lit.common.cfg
64 ↗(On Diff #20359)

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

This revision is now accepted and ready to land.Feb 20 2015, 7:51 AM
This revision was automatically updated to reflect the committed changes.