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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
