[PATCH] D97993: [Driver] Suppress GCC detection under -B for non-Android

[Nick Desaulniers via Phabricator via cfe-commits]

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