Page MenuHomePhabricator

[Support] Avoid normalization in sys::getDefaultTargetTriple
ClosedPublic

Authored by phosek on May 21 2018, 10:50 AM.

Details

Summary

The return value of sys::getDefaultTargetTriple, which is derived from
-DLLVM_DEFAULT_TRIPLE, is used to construct tool names, default target,
and in the future also to control the search path directly; as such it
should be used textually, without interpretation by LLVM.

Normalization of this value may lead to unexpected results, for example
if we configure LLVM with -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-linux-gnu,
normalization will transform that value to x86_64--linux-gnu. Driver will
use that value to search for tools prefixed with x86_64--linux-gnu- which
may be confusing. This is also inconsistent with the behavior of the
--target flag which is taken as-is without any normalization and overrides
the value of LLVM_DEFAULT_TARGET_TRIPLE.

Users of sys::getDefaultTargetTriple already perform their own
normalization as needed, so this change shouldn't impact existing logic.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.May 21 2018, 10:50 AM
delcypher added a comment.EditedMay 21 2018, 11:07 AM

I'm not familiar with this code but if I'm reading the patch correctly, then the commit message omits the fact that it changes the windows target triple from having a win32 suffix to windows-msvc suffix. I'm not sure what the implications are here but the fact you've done this is probably worth mentioning in the commit message.

phosek edited the summary of this revision. (Show Details)May 21 2018, 9:56 PM

I'm not familiar with this code but if I'm reading the patch correctly, then the commit message omits the fact that it changes he windows target triple from having a win32 suffix to windows-msvc suffix. I'm not sure what the implications are here but the fact you've done this is probably worth mentioning in the commit message.

Done. I've also tested this change on Windows and this seems to be passing now so should be ready for re-land.

Done. I've also tested this change on Windows and this seems to be passing now so should be ready for re-land.

Spoke too early, the following tests are still failing:

Clang :: Analysis/cxx-crashes.cpp
Clang :: CodeGen/2007-06-18-SextAttrAggregate.c
Clang :: CodeGen/2008-12-23-AsmIntPointerTie.c
Clang :: CodeGen/statements.c
Clang :: CodeGenCXX/2005-01-03-StaticInitializers.cpp
Clang :: CodeGenCXX/reinterpret-cast.cpp
Clang :: CodeGenCXX/vtable-debug-info.cpp
Clang :: Driver/coverage_no_integrated_as.c
Clang :: Driver/inhibit-downstream-commands.c
Clang :: Driver/linker-opts.c
Clang :: Driver/no-integrated-as.s
Clang :: Driver/nostdincxx.cpp
Clang :: Sema/array-init.c
Clang :: Sema/compound-literal.c
Clang :: SemaCXX/cstyle-cast.cpp
Clang :: SemaCXX/functional-cast.cpp
Clang :: SemaObjC/nonnull.m
Clang :: SemaObjCXX/pointer-to-objc-pointer-conv.mm
phosek updated this revision to Diff 148136.May 22 2018, 5:41 PM

I'm not familiar with this code but if I'm reading the patch correctly, then the commit message omits the fact that it changes he windows target triple from having a win32 suffix to windows-msvc suffix. I'm not sure what the implications are here but the fact you've done this is probably worth mentioning in the commit message.

Done. I've also tested this change on Windows and this seems to be passing now so should be ready for re-land.

Thanks. This code really isn't my area though so you should wait for a LGTM from one of your reviewers.

llvm/utils/lit/lit/llvm/config.py
253 ↗(On Diff #148136)

This comment probably needs updating. s/win32/windows/

phosek updated this revision to Diff 148240.May 23 2018, 10:15 AM
phosek marked an inline comment as done.
phosek added inline comments.May 23 2018, 2:51 PM
llvm/utils/lit/lit/llvm/config.py
252 ↗(On Diff #148240)

@rnk I assume this should only be the case when the triple is *-windows-msvc, not *-windows-gnu, but what if user omits the environment? Shall we just bail out or pick some default?

phosek updated this revision to Diff 148463.May 24 2018, 12:27 PM
rnk added a comment.May 24 2018, 1:49 PM

I think we can probably split this patch up to reduce the risk of breaking things. I think if you revert the config-ix.cmake change and just move triple normalization from LLVM to clang, everything should keep working. LLVM tools like llc might start to break in interesting ways, though, since they won't do the normalization that clang is doing. If check-llvm passes, though, I'm happy.

The config-ix.cmake and %itanium_abi_triple can be a follow-up.

phosek updated this revision to Diff 148538.May 24 2018, 6:42 PM
In D47153#1111541, @rnk wrote:

I think we can probably split this patch up to reduce the risk of breaking things. I think if you revert the config-ix.cmake change and just move triple normalization from LLVM to clang, everything should keep working. LLVM tools like llc might start to break in interesting ways, though, since they won't do the normalization that clang is doing. If check-llvm passes, though, I'm happy.

The config-ix.cmake and %itanium_abi_triple can be a follow-up.

All the tests seem to be passing now, but the patch is rather large now. I tried splitting up the the triple normalization and everything seems to be still passing so I'm fine landing those two parts separately.

phosek updated this revision to Diff 148634.May 25 2018, 11:03 AM
phosek edited the summary of this revision. (Show Details)
rnk accepted this revision.May 25 2018, 11:31 AM

lgtm

This revision is now accepted and ready to land.May 25 2018, 11:31 AM
This revision was automatically updated to reflect the committed changes.