MaskRay added inline comments.
================ 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. ---------------- nickdesaulniers wrote: > 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? Thanks for the feedback. Updated. ================ 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()); ---------------- nickdesaulniers wrote: > 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. Thank for the feedback that Android does not need this. I was unsure so I tried to keep Android unchanged. Seems that the special case is not actually needed. 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
