[cfe-commits] r149083 - in /cfe/trunk: include/clang/Driver/ToolChain.h lib/Driver/Driver.cpp lib/Driver/ToolChain.cpp lib/Driver/ToolChains.cpp lib/Driver/ToolChains.h lib/Driver/WindowsToolChain.cpp test/Driver/prefixed-tools.c test/Driver/pref

[Joerg Sonnenberger]

On Mon, Jan 30, 2012 at 06:17:17PM -0800, Chandler Carruth wrote:
> Reviewers and the code owner for Clang have already looked at this patch,
> and provided feedback that we need a different approach. The accepted path
> forward is to revert the patch, produce a new one addressing that feedback
> and discuss it on the mailing list.

Thanks for breaking it again. Can you at least revert the part of your
change again that created this mess? Moving back the FIXME for setting
up DefaultTargetTriple is all that is required...

> 1) It incorrectly prefers the "user" triple rather than trying to find an
> effective *toolchain* triple. This is really important if users ask for
> i686--linux-gnu, but we find an x86_64--linux-gnu toolchain that happens to
> support multiarch operation through the '-m32' flag.

I disagree with this reasoning. I already gave you examples why trying
to derive one GNUish triple from the other is bound to fail. The other
is there is absolutely no point in trying to play games like this in a
consistent toolchain setup. If you are doing cross-compiling and you
can't get the compiler name right, you have bigger issues. I still
maintain that auto-detection like that is not only unreliable, it makes
it a lot harder to find issues when (not if) they happen.

> 2) It forces every toolchain interface to carry the burden of tracking both
> triples, when instead the base classes can handle all of this logic.

If you read the patch, it is tracked in the base class (ToolChain). The
rest is just passing it down in the constructor chain.

> 4) It needlessly includes both triples in the cache key of the ToolChain.

Your detection "magic" is trying very hard to blur the distinction
between subarchitections in the toolchain. But the fact remains, that
targetting -m32 and -m64 are different things. Just because the
instances for now don't cache the choices they make, doesn't mean that
it doesn't make sense to do so. If there are ever supposed to be
different ToolChain instances at the same time, that breaks.

Right now I am not convinced that having multiple ToolChain entries
around is really going to work. There are a number of other elements
that can change the behavior of a ToolChain in critical ways and are
part of the driver, e.g. Sysroot.

Joerg