Page MenuHomePhabricator

[Driver][RISCV] Support parsing multi-lib config from GCC.
Needs RevisionPublic

Authored by kito-cheng on Mar 3 2021, 11:51 PM.

Details

Summary

RISC-V is highly configurable architecture, so it's hard to list all
possible multi-lib here, in order to deal with that issue , GCC has support
a flexible way to configure the multi-lib[1] for bare-matel toolchain.

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

[1] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=c1e6691245ca2f1f329549f323f67afe32bcb97a

Diff Detail

Event Timeline

kito-cheng created this revision.Mar 3 2021, 11:51 PM
kito-cheng requested review of this revision.Mar 3 2021, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 11:51 PM

Minor clean up

MaskRay added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
741

//

clang/lib/Driver/ToolChains/Gnu.cpp
1597
1627

camelCase. Just ignore older functions which use CamelCase.

clang/test/Driver/riscv64-toolchain.c
158 ↗(On Diff #328040)

Hmm. I happened to have posted https://lists.llvm.org/pipermail/cfe-dev/2021-March/067820.html about how folks use --gcc-toolchain.

Is this an useful option on non-riscv host?

kito-cheng added inline comments.Mar 4 2021, 12:43 AM
clang/test/Driver/riscv64-toolchain.c
158 ↗(On Diff #328040)

--gcc-toolchain should be useful for non-pure LLVM toolchain especially for bare-metal cross toolchain, IIRC GCC_INSTALL_PREFIX is not support relative path well like DEFAULT_SYSROOT[1], so I think this option is necessary even we have GCC_INSTALL_PREFIX cmake option.

[1] https://reviews.llvm.org/D76653

Address MaskRay's comment and apply clang-format.

kito-cheng marked 3 inline comments as done.Mar 4 2021, 12:55 AM
kito-cheng added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
741

Thanks, I guess some time my brain still in GCC development mode :P

Jim added inline comments.Mar 4 2021, 7:04 PM
clang/lib/Driver/ToolChains/Gnu.cpp
1705

Do you have plan to support other kind of options to build multilib?

Jim added inline comments.Mar 4 2021, 7:25 PM
clang/lib/Driver/ToolChains/Gnu.cpp
1746

scanGCCMultilibConfig ?

khchen added a comment.Mar 4 2021, 7:37 PM

I didn't check why I got error in
Failed Tests (1):

Clang :: Driver/riscv64-toolchain.c

Could you please double check it?
Thanks!

clang/lib/Driver/ToolChains/Gnu.cpp
1601

move into else

1606

if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_gcc_toolchain))

1627

bool -> static bool?
const std::string &MultilibOutput -> StringRef ?

The function name seem like it's not a target-specific function, but it's only support RISC-V now. Maybe we could rename it or make an assertion in function to check the target should be RISC-V?

1644

Could they be declared as StringRef and use llvm::StringSwitch<StringRef>?

1652
if (!File)
  return;
1738

rc->RC

1746

ScanGCCMultilibConfig->scanGCCMultilibConfig

Jim added a reviewer: Jim.Mar 4 2021, 9:20 PM
kito-cheng marked 5 inline comments as done.Mar 4 2021, 11:04 PM

Here is another solution for flexible multi-lib configuration I consider before:

  • Add an option called -fmultilib-config=
  • Using same or similar syntax with GCC's --with-multilib-generator

But the problem is it's really toooooo long for describe a multi-lib config.

e.g: current default bare-metal multi-lib configuration:
--with-multilib-generator="rv32i-ilp32--c;rv32im-ilp32--c;rv32iac-ilp32--;rv32imac-ilp32--;rv32imafc-ilp32f-rv32imafdc-;rv64imc-lp64--;rv64imfc-lp64f--;rv64imac-lp64--;rv64imafdc-lp64d--"

clang/lib/Driver/ToolChains/Gnu.cpp
1627

Maybe we could rename it or make an assertion in function to check the target should be RISC-V?

Good point, renamed.

1644

I would prefer keep const char * to prevent create a temporal object for StringRef here.

And I also found another place are used same way:

$ grep "StringSwitch<const" * -R
clang/lib/Basic/SourceManager.cpp:      llvm::StringSwitch<const char *>(BufStr)
clang/lib/Driver/ToolChains/Gnu.cpp:  std::string CurrentMCmodelOpt = llvm::StringSwitch<const char *>(CodeModel)
clang/lib/Driver/ToolChains/Clang.cpp:        MipsTargetFeature = llvm::StringSwitch<const char *>(Value)
clang/lib/Driver/ToolChains/Cuda.cpp:      OOpt = llvm::StringSwitch<const char *>(A->getValue())
clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:      OOpt = llvm::StringSwitch<const char *>(A->getValue())
clang/lib/Driver/ToolChains/Arch/Mips.cpp:    ABIName = llvm::StringSwitch<const char *>(CPUName)
clang/lib/Driver/ToolChains/Arch/Mips.cpp:    CPUName = llvm::StringSwitch<const char *>(ABIName)
clang/lib/Driver/ToolChains/Arch/PPC.cpp:    return llvm::StringSwitch<const char *>(CPUName)
clang/lib/Driver/ToolChains/Arch/PPC.cpp:  return llvm::StringSwitch<const char *>(Name)
clang/lib/Driver/ToolChains/Arch/Sparc.cpp:    return llvm::StringSwitch<const char *>(Name)
clang/lib/Driver/ToolChains/Arch/Sparc.cpp:    return llvm::StringSwitch<const char *>(Name)
clang/lib/Driver/ToolChains/Darwin.cpp:  return llvm::StringSwitch<const char *>(Arch)
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:        StringSwitch<const char *>(T.first->getValueAsString("AccessQualifier"))
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:      OS << StringSwitch<const char *>(
llvm/include/llvm/Support/FormatProviders.h:    Stream << StringSwitch<const char *>(Style)
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:          StringSwitch<const char *>(BroadcastString)
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:    const char *Repl = StringSwitch<const char *>(Name)
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:  const char *Repl = StringSwitch<const char *>(Op.getToken())
llvm/lib/Support/Host.cpp:  return StringSwitch<const char *>(StringRef(CPUStart, CPULen))
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/unittests/Support/Host.cpp:  StringRef MCPU = StringSwitch<const char *>(CPU)
1652

Good suggestion, thanks!

1705

Currently, no, but I think could be consider in future.

kito-cheng marked 2 inline comments as done.
  • Fix build issue.
  • Address Jim Lin's and Zakk's comment
jrtc27 added inline comments.Mar 5 2021, 4:57 AM
clang/test/Driver/Inputs/multilib_riscv32_elf_sdk/bin/riscv32-unknown-elf-gcc
2

Python 3

4–6

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.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
740

Nitpicking suggestion: \\ Default code model is 'small' (what GCC calls 'medlow').

clang/lib/Driver/ToolChains/Gnu.cpp
1659

Maybe trim whitespace before checking for empty lines, for extra robustness?

Address jrtc27's and luismarques comment

ChangeLogs

  • Move all new test to clang/test/Driver/riscv-toolchain-gcc-multilib.c
    • I don't found good way to test that on windows, since ExecuteAndWait only allow to execute .exe file on windows , upload a prebuilt *.exe file and execute that sounds like not reasonable.
  • Add error message output when execute GCC with non-zero return code, it come with a new test.
kito-cheng marked 8 inline comments as done.Mar 16 2021, 9:57 AM

This still doesn't report that the multilib configuration came from GCC when it succeeds, does it? I suppose that's not a deal-breaker, but it would be nice to have. Would it be difficult to implement?

Regarding the Windows test issue, aren't there other test cases in similar situations that managed to sort that out?

clang/lib/Driver/ToolChains/Gnu.cpp
1659

I was actually thinking something along the lines of Line = Line.trim();, so that the rest of the parsing code would also benefit from having any extraneous whitespace removed and thus be more robust. Or does this mutate the argument?

1742

configureation -> configuration. Also, the user won't know what the "non-zero return code" refers to. How about something like "Failed to execute <path> in an attempt to obtain the multilib configuration from GCC"?

This still doesn't report that the multilib configuration came from GCC when it succeeds, does it? I suppose that's not a deal-breaker, but it would be nice to have. Would it be difficult to implement?

There is some order issue is those function are executed before printing clang version info like clang version 13.0.0 (git@github.com:llvm/llvm-project.git 95cd87efe65d5c485670dd1281bf44dd0bff753d)..., some script might parse that, so I don't like to disrupt that, but I guess I could try to append verbose messages into MultilibErrorMessages and rename that to MultilibVerboseMessages.

kito-cheng updated this revision to Diff 331448.EditedMar 17 2021, 8:20 PM

ChangeLog:

  • Add more verbose message during reading GCC multilib configuration.
  • Add test case to verify verbose messages.
MaskRay requested changes to this revision.EditedMar 19 2021, 9:17 PM

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 (previously -### does not run programs). 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

About testing, multilib_riscv64_elf_sdk_bad{,2,3} suggest that lit tests are probably inadequate. You may need unit tests, see unittests/Driver/MultilibTest.cpp.

clang/include/clang/Basic/DiagnosticDriverKinds.td
395

Delete trailing period.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
742

getLastArgValue

clang/lib/Driver/ToolChains/Gnu.cpp
1602

--gcc-toolchain if specified

-gcc-toolchain is not a valid option.

1608

This should reuse the Generic_GCC::GCCInstallationDetector::init logic.

1661

This means an inlin element number of 128. This is excessive - consumes lots of stack space. The value should typically be <= 4.

clang/test/Driver/riscv-toolchain-gcc-multilib.c
6

This can pack more options on one line.

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

This revision now requires changes to proceed.Mar 19 2021, 9:17 PM
MaskRay added inline comments.Mar 19 2021, 9:21 PM
clang/lib/Driver/ToolChains/Gnu.cpp
1754

Even if ExecuteAndWait is accepted, you may consider a pipe. The current implementation leaves temporary files under /tmp.