Page MenuHomePhabricator

Make CMake choose the test exe target architecture according to the build.
ClosedPublic

Authored by zturner on Jul 30 2014, 2:29 PM.

Details

Reviewers
chandlerc
rnk
Summary

(I'm using chandlerc and rnk mostly for the CMake stuff, but naturally they are unfamiliar with the dotest stuff, so that's fair game if anyone else has any comments or concerns)

Previously, CMake was invoking the test runner and not specifying what architecture to use when building test executables. The Makefiles for the test executables then had logic to choose x64 by default. This doesn't work on Windows because the test compiler would then try to link against the 64-bit MSVCRT and not find them since only the 32-bit MSVCRT was in the path.

This patch addresses this by figuring out, at CMake time, whether or not you are building LLDB with a 64 or 32-bit toolchain. Then, it explicitly passes this value to the test runner, causing the test runner to build tests whose architecture matches that of LLDB itself. This can still be overridden by setting the CMake variable LLDB_TEST_EXECUTABLE_ARCH=(x64|x86)

Diff Detail

Event Timeline

zturner updated this revision to Diff 12047.Jul 30 2014, 2:29 PM
zturner retitled this revision from to Make CMake choose the test exe target architecture according to the build..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: chandlerc, rnk.
zturner added a subscriber: Unknown Object (MLST).
chandlerc added inline comments.Jul 30 2014, 4:01 PM
CMakeLists.txt
22–26 ↗(On Diff #12047)

So, this isn't the "native" architecture. For an LLVM cross compiler, it will not execute on the local machine at all. It's just the target architecture.

I don't think you want to have an 'LLDB_TARGET_ARCH' variable. That doesn't really make sense, especially in the singular.

I think you should just set this in the test-specific cmake, and I think you should make it 'LLDB_TEST_TRIPLE' which is cached, user-overridable, and defaults to 'LLVM_DEFAULT_TARGET_TRIPLE'.

test/CMakeLists.txt
20

Why isn't this just part of the COMMON_TEST_ARGS below?

22–23

If this is overridable then it should be cached with a descriptive string...

45

... and we shouldn't clobber it.

zturner added inline comments.Jul 30 2014, 4:40 PM
CMakeLists.txt
22–26 ↗(On Diff #12047)

LLDB doesn't support cross-compilation anyway, so I think it's safe to assume that the host and target are the same.

All we're trying to figure out, ultimately, is whether to pass -m32 or -m64 to clang. So allowing the user to specify a full triple seems like overkill and is likely to be confusing, it seems more straightforward if they just pass in "x86" or "x64".

zturner updated this revision to Diff 12069.Jul 31 2014, 9:55 AM

Fixed the variable caching and other cleanup

zturner updated this revision to Diff 12071.Jul 31 2014, 9:58 AM

Trying again, the correct patch didn't get attached last time.

rnk edited edge metadata.Jul 31 2014, 10:32 AM

Seems fine to me, but I'll let Chandler respond.

CMakeLists.txt
22–26 ↗(On Diff #12047)

Just doing the architecture seems reasonable for now. The triple could be useful if we ever want to support running the test suite in a way that is remotely debugging binaries in a VM with a different OS or something. That's probably either not useful or a long way off.

test/CMakeLists.txt
22

nit: s/STRING/string/, we appear to prefer lower-case cmake function names.

zturner updated this revision to Diff 12073.Jul 31 2014, 10:46 AM
zturner edited edge metadata.

Fix some issues with overriding one of the CMake variables, and other minor cleanup.

chandlerc accepted this revision.Jul 31 2014, 11:55 AM
chandlerc edited edge metadata.

Minor comment tweak. Otherwise this looks good now. Thanks.

I guess Greg wants you to wait for Todd's LGTM? :: shruug :: I know nothing about LLDB, just cmake. =]

test/CMakeLists.txt
18–19

This comment isn't quite accurate... Maybe:

"The default architecture with which to compile test executables is the default LLVM target architecture, which itself defaults to the host architecture."

This revision is now accepted and ready to land.Jul 31 2014, 11:55 AM
tfiala added a subscriber: tfiala.Jul 31 2014, 1:27 PM

I'll have a peek when I get to a computer (on vaca...)

zturner closed this revision.Aug 12 2014, 1:11 PM

This was already submitted