phosek added a subscriber: hvdijk.
phosek added a comment.
In D110663#3270509 <https://reviews.llvm.org/D110663#3270509>, @MaskRay wrote:
> Ping @phosek
I apologize about the belated response, but I wanted to test this change first
given the potentially large blast radius.
To clarify, when I talk Fuchsia toolchain, I don't just mean toolchain
targeting Fuchsia, but a Clang toolchain distribution our team manages that's
also used by a number of other teams such Dart, Flutter, Pigweed to build code
for variety of platforms including Linux.
In my testing, I found that these users aren't currently using a single uniform
target triple spelling, I've seen both `${arch}-linux-gnu` and
`${arch}-unknown-linux-gnu` used pretty liberally. So if we were to land this
change today, it would break many of these users. We could clean up and unify
all of these uses, but it's going to take some effort because it spans lots of
projects. I'm also worried that there might be other Clang users in a similar
situation who are not included on this change.
That's why I think there should be an RFC sent to cfe-dev, to both reach an
agreement on whether this is a direction everyone agrees with, but also to
raise the awareness. There's also a question whether this should be rolled out
in a more gradual fashion (for example by starting with a warning first).
Furthermore, I'm also not sure about the motivation behind this change. The
change summary mentions D109837 <https://reviews.llvm.org/D109837>, but @hvdijk
said that this is not a blocker. Is there another reason for doing this?
================
Comment at: clang/lib/Driver/Driver.cpp:428
+ const Driver &D, StringRef TargetTriple, const ArgList &Args,
+ llvm::Triple *UnnormalizedTriple = nullptr, StringRef DarwinArchName = "")
{
// FIXME: Already done in Compilation *Driver::BuildCompilation
----------------
I find the name `UnnormalizedTriple` a bit confusing as it may imply that
there's some "un-normalization" process involved. I'd prefer using a
`TargetTriple` to refer to "triple given by the user (via the `--target=`
option)" and `NormalizedTriple` as "normalized triple".
================
Comment at: clang/lib/Driver/Driver.cpp:1203-1206
+ if (Normalized.isOSFuchsia())
+ TargetTriple = Normalized.str();
+ else
+ TargetTriple = Unnormalized.str();
----------------
I really think this logic belongs to `clang::driver::toolchains::Fuchsia`.
Moving it there is going to require some changes to how toolchains are
instantiated, and should thus be done as a separate change, but doing so could
also help us cleanup other special case logic like that exists in `Driver` like
https://github.com/llvm/llvm-project/blob/8ba9c794feb30cd969b9776c39873def10c51bff/clang/lib/Driver/Driver.cpp#L576.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110663/new/
https://reviews.llvm.org/D110663
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
