This is an archive of the discontinued LLVM Phabricator instance.

[clang-cl] Fix PCH tests to use x86_64 as target
ClosedPublic

Authored by rovka on Aug 2 2016, 1:13 AM.

Details

Summary

These tests require x86-registered-target, but they don't force the target as
x86 on the command line, which means they will be run and they might fail when
building the x86 backend on another platform (such as AArch64).

Fixes PR28797

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 66439.Aug 2 2016, 1:13 AM
rovka retitled this revision from to [clang-cl] Fix 2 pch tests to use x86_64 as target.
rovka updated this object.
rovka added a reviewer: thakis.
rovka added subscribers: cfe-commits, hans.
rovka added a comment.Aug 2 2016, 1:16 AM

This seems to be the case for other similar tests as well (e.g. cl-pch-errorhandling.cpp requires x86-registered-target, but doesn't force it on the command line), but they're not currently crashing. I can update those as well if people think it's the right thing to do.

All tests should have the target explicit, so I'd advocate for all of them to be changed.

Also, we need to make sure --target=x86_64 is enough on Windows. I remember some -target=arm not being enough (arm-eabi was enough, for some reason).

rengolin updated this object.Aug 2 2016, 4:23 AM
rovka added a comment.Aug 2 2016, 6:06 AM

All tests should have the target explicit, so I'd advocate for all of them to be changed.

Coming right up...

Also, we need to make sure --target=x86_64 is enough on Windows. I remember some -target=arm not being enough (arm-eabi was enough, for some reason).

It's enough, I've seen it used in other clang-cl tests.

rovka updated this revision to Diff 66467.Aug 2 2016, 6:49 AM
rovka retitled this revision from [clang-cl] Fix 2 pch tests to use x86_64 as target to [clang-cl] Fix PCH tests to use x86_64 as target.

Added cl-pch-errorhandling.cpp

thakis accepted this revision.Aug 2 2016, 6:50 AM
thakis edited edge metadata.
This revision is now accepted and ready to land.Aug 2 2016, 6:50 AM
This revision was automatically updated to reflect the committed changes.