This is an archive of the discontinued LLVM Phabricator instance.

[llvm-rc] Don't rewrite the arch in the default triple unless necessary
ClosedPublic

Authored by mstorsjo on Jun 19 2021, 12:05 AM.

Details

Summary

When the default target arch isn't one that is supported as a
windows target, we want to set a suitable architecture (so that
Clang tests that run plain 'llvm-rc' succeed checks for e.g.
"#ifdef _WIN32" even for llvm builds that default to e.g. ppc64).

But if the default target architecture is usable, don't rewrite it.
(Rewriting it, by e.g. "T.setArch(T.getArch())", normalizes the
spelling of the architecture, e.g. changing i686 to i386. Such a
change can make clang unable to find the right sysroot.)

This can't, unfortunately, practically be tested very well because
it is entirely dependent on the default triple of the llvm build.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Jun 19 2021, 12:05 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2021, 12:05 AM

The idea looks fine. Just a couple inline questions.

llvm/tools/llvm-rc/llvm-rc.cpp
148

Should this be just isUsableArch? It doesn't seem like Default is relevant in this new formulation.

184

Is there a guarantee that the fallback will be usable?

mstorsjo added inline comments.Jun 22 2021, 11:05 AM
llvm/tools/llvm-rc/llvm-rc.cpp
148

Sure, I guess I could leave that word out

184

The main criterion for usable here practically is whether “clang -target <arch>-windows -E” works as expected, i.e. preprocesses with _WIN32 defined. Clang does handle that for those four arches even if the corresponding target lowering backend is disabled.

Or put another way, we’ve got tests within clang that try to do preprocessing by just calling llvm-rc or llvm-windres without specifying any target arch, and checking that the preprocessing works a as expected (defining _WIN32 and RC_DEFINED - the latter comes from a parameter passed from llvm-rc). If llvm defaults to e.g. ppc64, then we must override the arch and do the preprocessing test with e.g. an x86_64 triple.

mstorsjo updated this revision to Diff 353749.Jun 22 2021, 12:20 PM

Renamed isUsableDefaultArch to isUsableArch

amccarth accepted this revision.Jun 25 2021, 10:00 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Jun 25 2021, 10:00 AM
This revision was landed with ongoing or failed builds.Jun 25 2021, 12:59 PM
This revision was automatically updated to reflect the committed changes.