[PATCH] D97916: [Driver][RISCV] Support parsing multi-lib config from GCC.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

In D97916#2625170 <https://reviews.llvm.org/D97916#2625170>, @luismarques wrote:

> This patch seems to be in pretty good shape now.
>
> One thing that might be useful (important?) to add is additional diagnostics when run in verbose mode. Currently `clang -v` will indicate that it found the GCC installation and will list the multilibs but there will be no indication that the multilib list came from GCC. Also, if things like the `ExecuteAndWait` (in `getRISCVMultilibFromGCC`) fail shouldn't we print some kind of diagnostic, at least in verbose mode? Otherwise when problems occur it might be tricky to figure out what's going on.

Running an external program (`$triple-gcc`) to get library/include paths? This is a very unusual behavior to me and can have security issues. I'd suggest you raise a topic on cfe-dev getting consensus before this can be committed.

In the test, clang will invoke `python3 Inputs/multilib_riscv64_elf_sdk/bin/riscv64-unknown-elf-gcc --print-multi-lib` and parse its output - this is unusual, too. I am concerned whether we can do this.

> So I think it would be great to just leverage that from GCC, the simplest way is asking the multi-lib configuration from GCC.

The description should include an overview about what the patch does, not simply deferring to another place about what it will do. By following the link - the description does not clearly state what the particular GCC commit does as well.

Only by following the logic under a debugger it becomes clear to me that the patch adds code to spawn an external process of gcc, parse its output, and uses that as include/library/march/etc.

As an alternative, have you considered https://clang.llvm.org/docs/UsersManual.html#configuration-files



================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:395
+  "%0 option unrecognized in multi-lib configuration when parsing config "
+  "from GCC, falling back to built-in multi-lib configuration.">,
+  InGroup<MultilibFallback>;
----------------
Delete trailing period.


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:742
+  StringRef CodeModel;
+  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
+    CodeModel = A->getValue();
----------------
getLastArgValue


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1602
+
+  // Find GCC from -gcc-toolchain if given.
+  if (const Arg *A =
----------------
`--gcc-toolchain` if specified

`-gcc-toolchain` is not a valid option.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1608
+    // Try to find GCC from GCC_INSTALL_PREFIX if define.
+    llvm::StringRef GCCInstallPrefix = GCC_INSTALL_PREFIX;
+    std::string GCCPath;
----------------
This should reuse the `Generic_GCC::GCCInstallationDetector::init` logic.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1661
+
+  SmallVector<StringRef, 128> Lines;
+  File.get()->getBuffer().split(Lines, "\n");
----------------
This means an inlin element number of 128. This is excessive - consumes lots of stack space. The value should typically be <= 4.


================
Comment at: clang/test/Driver/riscv-toolchain-gcc-multilib.c:6
+
+// RUN: %clang %s \
+// RUN:   -target riscv32-unknown-elf \
----------------
This can pack more options on one line.

Having more lines makes the file longer and actually harms readability.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97916/new/

https://reviews.llvm.org/D97916