[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

[Chandler Carruth]

On Fri, Jan 27, 2012 at 4:21 PM, Joerg Sonnenberger <joerg at britannica.bec.de
> wrote:

> On Fri, Jan 27, 2012 at 03:16:17PM -0800, Douglas Gregor wrote:
> > > Reverting just this change is inacceptable. It fixes a major regression
> > > for cross-compiling support from your earlier changes, without
> > > introducing a bag of new ones.
> >
> > There is a legitimate concern that your change is not the right
> > direction. Please revert, and we can discuss how best to implement
> > this functionality.
>
> That doesn't answer the question of how to deal with the original change
> that broke it. So before randonly reverting changes -- where should the
> user triple belong?
>

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.

In this case, the patch never got discussed at all before committing, and
so it would seem like a good place for you to start would be to revert and
submit exactly this patch to the mailing list for review where we can work
through any problems with it.


In any event, I've reverted this patch according to Doug's request in
rNNNNN.

I'm working on an alternate design since I am interested in the use case,
and we still don't have a proposed patch for proper code review where
design discussions can take place, but I'll respond here at a high level to
these items:

(a) Driver. Old location before the driver refactoring with FIXME as it
> required parsing the arguments.
>

It was also never intended ta capture the *user*'s triple, or anything
other than the *default* triple. Relying on that was relying on an accident
of the code, and one which nothing else used or enforced.

(b) Toolchain. That's what implemented in this change. This also allows
> platforms to deal with more limited toolchains, e.g. old as doesn't
> support --32 if built only for i386.
>

This is where the functionality belongs, but your patch doesn't implement
it there in a clean manner. I've outlined some of the issues in the revert
commit log, but to repeat them here:

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.

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.

3) It uses very confusing naming in the Driver change, with "Target" and
"TargetTriple" both naming triples, but not the same triple, and one of
them not the target triple.

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


Anyways, we should continue this discussion on proposed patches rather than
on the commit log for this one. I hope to have the first patch in the
series out quite soon.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120130/a6065c94/attachment.html>