nickdesaulniers requested changes to this revision. nickdesaulniers added a comment. This revision now requires changes to proceed.
Let's drop the Android part, too? Update the description (commit message), too? I checked our oldest supported kernel version, and we don't use `-B` either: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Makefile?h=v4.4.262#n606 ================ Comment at: clang/docs/ReleaseNotes.rst:75 - -Wshadow now also checks for shadowed structured bindings +- ``-B <prefix>`` (when ``<prefix>>`` is a directory) used to detect GCC + installations under ``<prefix>``. This behavior is incompatible with GCC, ---------------- is there an extra `>` in the second occurrence of `prefix` on this line? ================ Comment at: clang/docs/ReleaseNotes.rst:76-77 +- ``-B <prefix>`` (when ``<prefix>>`` is a directory) used to detect GCC + installations under ``<prefix>``. This behavior is incompatible with GCC, + causes interop issues with ``--gcc-toolchain``, and is thus dropped. Specify + ``--gcc-toolchain=<prefix>`` instead. ---------------- The first time I read `This behavior is incompatible with GCC, causes interop issues with ``--gcc-toolchain``, and is thus dropped.` I thought you were dropping the `-B` flag entirely. I see now you're referring to `This behavior` being dropped, but maybe this can be reworded to avoid confusing others? ================ Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1911 + SmallVector<std::string, 8> Prefixes; + if (TargetTriple.isAndroid()) + Prefixes.assign(D.PrefixDirs.begin(), D.PrefixDirs.end()); ---------------- MaskRay wrote: > srhines wrote: > > danalbert wrote: > > > I'm not entirely sure what `D.PrefixDirs` represents so maybe Android > > > doesn't need this either. > > > > > > The behavior the NDK depends on is being able to find tools co-located > > > with the Clang driver location. Aside from `as`, these are all LLVM tools > > > (lld and co). > > > > > > The sysroot is expected to be in `$CLANG/../sysroot`. All our headers, > > > libraries (aside from libgcc/libatomic), and CRT objects are located > > > there. > > > > > > The clang driver install location is expected to also be a GCC install > > > directory, so libgcc/libatomic are expected at > > > `$CLANG/../lib/gcc/$TRIPLE/$GCC_VERSION`. > > > > > > Typical usage for the NDK does not involve `-gcc-toolchain` or `-B` at > > > all. > > > > > > If I've understood correctly, your change can be applied to Android as > > > well without breaking any of those behaviors. @srhines will need to > > > comment on whether the Android platform build needs this, but aiui anyone > > > depending on this behavior just needs to fix their build to use > > > `-gcc-toolchain` where they were previously using `-B`. > > > > > > Of course, I can't speak to what our users with custom build systems that > > > don't follow our docs might be doing. > > From a look at Android's build system, we are using `-B` but we don't > > currently use `--sysroot` or `-gcc-toolchain` for platform builds. I did > > try a build with this patch (but not retaining the Android-specific part), > > and it still worked fine. From looking at the constructed command lines, > > the platform build generally adds the appropriate include paths, library > > paths, and invokes separate tools directly, so we aren't relying on `-B` > > for this purpose. Cleaning this up is on my list of things to eventually > > do, but as I see it, we're not going to be negatively impacted even with > > the more aggressive version of this patch. > > > > > Of course, I can't speak to what our users with custom build systems that > > > don't follow our docs might be doing. > > I do share this concern a bit, but it's very hard for me to quantify how > > hard it would be for someone to just adapt their build if we did make a > > more aggressive change here. If it's really as easy as passing > > `-gcc-toolchain`, then perhaps that would be fine for the Release Notes. > It should be as easy as passing `--gcc-toolchain`. I edited the summary to > state more clearly that the current behavior actually makes interop with > `--gcc-toolchain` difficult. > > I'll not touch Android to restrict the influence to regular Linux > distributions in this patch. > Android can be fixed separately (which require two test changes.) > I did try a build with this patch (but not retaining the Android-specific > part), and it still worked fine. Then we should drop this Android specific part. If @srhines tested it without, there's no issue. > From a look at Android's build system, we are using -B but we don't currently > use --sysroot or -gcc-toolchain for platform builds We're probably mostly using LLVM utils anyways. > It should be as easy as passing --gcc-toolchain. Yep. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97993/new/ https://reviews.llvm.org/D97993 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
