This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Collect library directories and triples for riscv64 triple too
ClosedPublic

Authored by edward-jones on Oct 18 2018, 5:36 AM.

Details

Summary

When setting up library and tools paths when detecting an accompanying GCC installation only riscv32 was handled. As a consequence when targetting riscv64 neither the linker nor libraries would be found. This adds handling and tests for riscv64.

Diff Detail

Event Timeline

edward-jones created this revision.Oct 18 2018, 5:36 AM
jrtc27 added inline comments.Oct 18 2018, 9:54 AM
lib/Driver/ToolChains/Gnu.cpp
1911–1912

Surely this should remain as RISCV32LibDirs? Also, should we reverse the order (put "/lib32" first) to match every single other architecture?

1911–1912

RISCV32Triples? Also, why do we have no "riscv32-linux-gnu" entry like the other architectures?

1913

Order as for 32.

1914

riscv64-linux-gnu?

jrtc27 added inline comments.Oct 18 2018, 10:02 AM
test/Driver/riscv64-toolchain.c
71

This (and below) are the only instances of CPU-linux-unknown-elf in the whole of clang (and there are only two instances of CPU-linux-elf, both in test/Modules/pch_container.m), apart from the riscv32 ones they were copied from. Perhaps they should be riscv64-linux-unknown-gnu instead? Notably, when parsed as CPU-company-kernel-OS this in fact sets *company* to linux and *kernel* to unknown, whereas if you really mean the full 4-tuple should the linux and unknown not be interchanged?

87

riscv64-linux-unknown-gnu as before.

I've incorporated your suggested changes and added riscv32/64-linux-gnu entrys to the Triple + LibDirs lists.

Is it worth also updating the riscv32-toolchain.c test in this patch to rename riscv32-linux-unknown-elf to riscv32-unknown-linux-gnu? It looks like "riscv32-linux-unknown-elf" has been there since the the driver was introduced.

edward-jones marked 2 inline comments as done.
edward-jones marked 4 inline comments as done.Oct 22 2018, 1:24 AM
asb accepted this revision.Apr 4 2019, 7:15 AM

This got missed somehow as I had a functionally identical patch in my local development tree (though not with as thorough tests - thanks for that!). LGTM, thanks Ed.

This revision is now accepted and ready to land.Apr 4 2019, 7:15 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 7:18 AM