This is an archive of the discontinued LLVM Phabricator instance.

Not sure lld is the default? I think I always build with bfd (or gold).
I'll check later today when I'm in the office. I'm not against this change per se, but adding another dependency seems annoying. cc: @zturner

@davide: I agree, the extra dependency is annoying.

FWIW, I was trying to run tests on Windows and nothing worked because lld wasn't built. Maybe the behavior is different on Windows and on other hosts? I just assumed this was the new default because I saw a couple of changes to how tests are run recently.

lld is required for building windows inferiors, but that can't be the case elsewhere (I don't even have lld checked out). It's possible it gets somehow auto-selected by clang when you have lld checked out, but if that's the case than you need to detect it, and enable the dependency only then.

This revision now requires changes to proceed.Nov 6 2017, 9:31 AM

Check only on Windows.

labath accepted this revision.Nov 6 2017, 9:53 AM

This revision is now accepted and ready to land.Nov 6 2017, 9:53 AM

Do we want to add an option to our build system to try LLD where it is supported? Doesn't need to be part of this patch, but it would be great to be able to try it out on ELF based systems.

zturner accepted this revision.Nov 6 2017, 10:22 AM

zturner added inline comments.

test/CMakeLists.txt
116–122	I would probably move this outside of the `if (TARGET clang)`. This is minor though, as I can think of ways to break this both with the current patch (inside of the check) as well as outside of the check.

Move check out of if (TARGET clang) block.

Closed by commit rL317501: Add a dependency from check-lldb on lld (authored by sas). · Explain WhyNov 6 2017, 11:26 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

test/

CMakeLists.txt

9 lines

Diff 121760

test/CMakeLists.txt

Show First 20 Lines • Show All 107 Lines • ▼ Show 20 Lines	add_python_test_target(check-lldb
"-q;${LLDB_DOTEST_ARGS}"		"-q;${LLDB_DOTEST_ARGS}"
"Testing LLDB (parallel execution, with a separate subprocess per test)"		"Testing LLDB (parallel execution, with a separate subprocess per test)"
)		)

# If we're building with an in-tree clang, then list clang as a dependency		# If we're building with an in-tree clang, then list clang as a dependency
# to run tests.		# to run tests.
if (TARGET clang)		if (TARGET clang)
add_dependencies(check-lldb clang)		add_dependencies(check-lldb clang)
endif()		endif()

		# LLD is required to link test executables on Windows.
		if (CMAKE_SYSTEM_NAME MATCHES "Windows")
		if (TARGET lld)
		add_dependencies(check-lldb lld)
		else ()
		zturnerUnsubmitted Not Done Reply Inline Actions I would probably move this outside of the `if (TARGET clang)`. This is minor though, as I can think of ways to break this both with the current patch (inside of the check) as well as outside of the check. zturner: I would probably move this outside of the `if (TARGET clang)`. This is minor though, as I can…
		message(WARNING "lld required to test LLDB on Windows")
		endif ()
		endif ()

add_custom_target(lldb-test-depends DEPENDS ${LLDB_TEST_DEPENDS})		add_custom_target(lldb-test-depends DEPENDS ${LLDB_TEST_DEPENDS})
# This will add LLDB's test dependencies to the depenednecies for check-all and		# This will add LLDB's test dependencies to the depenednecies for check-all and
# include them in the test-depends target.		# include them in the test-depends target.
set_property(GLOBAL APPEND PROPERTY LLVM_LIT_DEPENDS ${ARG_DEPENDS})		set_property(GLOBAL APPEND PROPERTY LLVM_LIT_DEPENDS ${ARG_DEPENDS})

This is an archive of the discontinued LLVM Phabricator instance.

Add a dependency from check-lldb on lldClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 121760

test/CMakeLists.txt

Add a dependency from check-lldb on lld
ClosedPublic