This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][Driver] Fix baremetal `GCCInstallation` paths
Needs RevisionPublic

Authored by anton-afanasyev on Jun 27 2022, 1:21 AM.

Details

Summary

In general gcc toolchain folder contains several targets, for instance,
baremetal and Linux ones. The precommitted tests address this case by
adding riscv64-unknown-linux-gnu/ folder to toolchain. This breaks driver's
include and lib paths, since riscv baremetal triple is now normalized to
"riscv{32|64}-unknown-unknown-elf" rather than just to "riscv{32|64}-unknown-elf",
and GCCInstallation uses this search priority: "riscv{32|64}-unknown-linux-gnu",
"riscv{32|64}-linux-gnu", "riscv{32|64}-unknown-elf", choosing Linux target.

So withouth this patch driver makes incorrect mix of completely different target triples in paths.
This patch fixes this issue by passing triple alias for baremetal target.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 1:21 AM
anton-afanasyev requested review of this revision.Jun 27 2022, 1:21 AM
anton-afanasyev edited the summary of this revision. (Show Details)Jun 27 2022, 5:03 AM
anton-afanasyev edited the summary of this revision. (Show Details)
kito-cheng accepted this revision.Jun 29 2022, 7:26 PM

LGTM, riscv*-unknown-unknown match riscv*-unknown-elf sound make more sense than riscv*-unknown-linux-gnu :)

This revision is now accepted and ready to land.Jun 29 2022, 7:26 PM
MaskRay requested changes to this revision.Jun 29 2022, 7:45 PM
This revision now requires changes to proceed.Jun 29 2022, 7:45 PM

The description seems unclear to me. Is a riscv64-unknown-linux-gnu GCC installation selected while the requested target triple is riscv64-unknown-elf?
This could be an instance of https://discourse.llvm.org/t/rfc-fix-loose-behaviors-of-clang-target/60272 ([RFC] Fix loose behaviors of Clang –target=) and the right fix may be somewhere upper level.

The description seems unclear to me. Is a riscv64-unknown-linux-gnu GCC installation selected while the requested target triple is riscv64-unknown-elf?

Yes, exactly (see precommitted tests 4ee6b7806bc0: adding one more target directory confuses driver, mixing triples in paths). Adjusting description to be more clear.

This could be an instance of https://discourse.llvm.org/t/rfc-fix-loose-behaviors-of-clang-target/60272 ([RFC] Fix loose behaviors of Clang –target=) and the right fix may be somewhere upper level.

This looks related but not the same one: this patch fixes the incorrect use of completely different targets. I believe this issue could be addressed on top of your RFC implemented, need to refactor triple, setting equivalence of triples explicitly. Anyway, I'd like to have this patch committed meanwhile: it is more related to how RISCVToolchain uses baremetal GCCInstallation for the current state.

anton-afanasyev edited the summary of this revision. (Show Details)Jun 30 2022, 2:04 AM
arichardson added inline comments.Jun 30 2022, 3:03 AM
clang/lib/Driver/ToolChains/RISCVToolchain.cpp
54

This seems like the wrong place to add this workaround, shouldn't the change be in GCCInstallation::init? That way targets other than RISC-V also benefit from this fix.

anton-afanasyev marked an inline comment as done.Jun 30 2022, 6:23 AM
anton-afanasyev added inline comments.
clang/lib/Driver/ToolChains/RISCVToolchain.cpp
54

GCCInstallation knows nothing about triple equivalence of the specific targets, but provides TripleAliases init variable for installation target callers. Other targets are to do the same initialization as we do here in case of different normalized and layout-used triple names.

yakush added a subscriber: yakush.Jul 1 2022, 2:58 AM
arichardson added inline comments.Jul 3 2022, 12:21 PM
clang/lib/Driver/ToolChains/RISCVToolchain.cpp
54

What I was trying to say is that the normalization adding -unknown affects all targets, so maybe GCCInstallation::init() should add aliases that have the -unknown components removed rather than doing this at the call site. This way the alias computation does not need to be moved to each target info.

However, I also think we should be passing through the triple as passed to --target= rather than the normalized triple.