The BareMetal.cpp part looks ok to me.

In D138195#3937618, @jroelofs wrote:

The newlib + libc++ work I did for a previous employer (CodeSourcery/MentorGraphics/Siemens) was aimed at building a clang toolchain that could support building for baremetal arm. An ABI break for that environment would not be the end of the world, especially considering those environments cannot afford the code size from locale support. AFAIK, @abidh is still maintaining that work, and would be able to give a definitive answer.

In D136712#3892986, @manojgupta wrote:

@tomhughes has more details on this but if we do not define it in clang itself, we'll need to define it for every package manually at build time since the usage goes deep inside newlib headers.

In D131225#3706239, @barannikov88 wrote:

In D131225#3706207, @abidh wrote:

I think this patch is moving the things in the right direction. But please wait for some other reviewers to chime in.

It seems quite opposite to me.

I think this patch is moving the things in the right direction. But please wait for some other reviewers to chime in.

Committed with slight whitespace change to keep clang-format happy.

In D119837#3326179, @asb wrote:

Thanks Kito. This LGTM, but I'd appreciate someone else reviewing and approving as well.

Handle review comments.

Rebase. One of the test file has moved in the repo so updated the patch accordingly.

Remove else after the return.

You need to add some tests that check the functionality that you are adding. Look at clang/test/Driver/baremetal.cpp for an example.

LGTM

If gcc and clang based toolchains are installed in the same prefix then having sysroot at same location can cause one installation to overwrite parts of other.

In D98113#2609027, @jrtc27 wrote:

One thing that is currently rather ugly about BareMetal is that it ignores the AddArch argument. Once you have a sysroot, the architecture suffix is rather unnecessary, and given compiler-rt uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide both whether to add a suffix and whether to add lib/${OS} to the install path, that means you can't nicely point compiler-rt at the sysroot as the install directory (well, unless you do something like make COMPILER_RT_OUTPUT_DIR point at the sysroot not the libdir and set COMPILER_RT_OS_DIR to . (or maybe the empty string also works)).

Changes in this version.

1. Added an explicit --sysroot argument.
2. Shortened "testroot-riscv{len}-baremetal-nogcc" string

Rebase.

Handle review comments.

It seems that testcase I added has brought to surface a hidden issue. If you
look at the end of Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes,
it adds sysroot/usr to the Prefixes. If the sysroot is empty, this will end up
using the toolchain that you installed in /usr.

Committed in 1deff4009e0a on request of @burnus.

In D93023#2446757, @MaskRay wrote:

If you do this, testroot-riscv64-baremetal-nogcc/ is probably also not useful (or you can replace it with a shorter string`.

Can you fix the tests when gcc-riscv64-linux-gnu is installed?

Remove an unintended change.

In D91442#2444399, @MaskRay wrote:

This change has been failing on my machine for the past two weeks.

riscv32-toolchain-extra.c and riscv64-toolchain-extra.c are not hermit. I have installed /usr/lib/gcc-cross/riscv64-linux-gnu/10 (gcc-10-riscv64-linux-gnu and its dependencies) on my machine and the test will find some directories relative to /usr/lib/gcc-cross/riscv64-linux-gnu/10. I think --sysroot is required for such tests.

%T is a deprecated lit feature. Please do something like rm -rf %t; mkdir %t instead.

Updated other instances in llvm/docs. I have initially updated the whole tree but that becomes quite a big patch. I can post that one if it is of interest.

In D92831#2442243, @kristof.beyls wrote:

Thanks for this.
The proposed changes all LGTM.
I did do a grep on the term 'master' in llvm/docs, and found quite a few more uses of 'master', especially in links.
The command I used was: grep -R master llvm/docs/
Just two examples are:

llvm/docs/CodingStandards.rst:<https://github.com/llvm/llvm-project/tree/master/llvm/include/llvm/ADT>`_)
llvm/docs/GitBisecting.rst:being able to reset, do the usual ``git checkout -f master; git reset --hard

Would you also be willing to adapt/fix the URLs pointing to llvm-project in that directory?

Thanks,

Kristof

Addressed review comments.

Handled review comments.

ping.

@jroelofs Do you have any comments on this patch?

Hi @lenary,
Do you any more comments on the patch?

This update contains following changes.

Handle review comment.

In D91442#2399772, @lenary wrote:

I recall that there is also https://reviews.llvm.org/D68407 which iirc hoped to address using RISCVToolchain with Compiler-rt and libunwind - presumably it is not a complete solution, but it might be we want to use some checking of the GCCInstallation (like RISCVToolChain::GetDefaultRuntimeLibType does) to choose which toolchain to instantiate? I'm not sure if we can satisfy that ordering though.

In D91442#2399753, @jrtc27 wrote:

In D91442#2399750, @abidh wrote:

In D91442#2399402, @lenary wrote:

In D91442#2399341, @abidh wrote:

In D91442#2399200, @lenary wrote:

I'm worried about this change - I *think* it doesn't cover the existing behaviour of a baremetal GCC toolchain being installed into the same prefix as clang, and clang automatically picking up that baremetal gcc toolchain. What should we expect to do here? This is especially an issue if you're trying to make a relocatable toolchain tarball, where specifying --gcc-toolchain automatically is difficult.

Would it be possible to use a relative path with --gcc-toolchain then this can be checked in either RISCVToolChain.cpp or GNU.cpp and adjusted accordingly?

The GCC toolchain, when given a relative path, already interprets it relative to the working directory the compiler was invoked from, not the directory the compiler is located in, iirc.

I dont think path is made absolute relative using working directory. What is happening is that if (!VFS.exists(Prefix)) call in the following line will succeed if the path existed relative to working directory. This seems accidental to me.

https://github.com/llvm/llvm-project/blob/5a9f3867046c4e1c97760e22a505f4d1d788417e/clang/lib/Driver/ToolChains/Gnu.cpp#L1947

I tried a small change to turn a relative --gcc-toolchain into absolute using the compiler installed path and it works ok. If this is something that works for you then I can post a patch.

Relative paths should be relative to the current working directory. That is what everyone expects and is what every other argument does (and see = and $SYSROOT for how GCC and compatible drivers get around that in cases like -I).

In D91442#2399402, @lenary wrote:

In D91442#2399341, @abidh wrote:

In D91442#2399200, @lenary wrote:

I'm worried about this change - I *think* it doesn't cover the existing behaviour of a baremetal GCC toolchain being installed into the same prefix as clang, and clang automatically picking up that baremetal gcc toolchain. What should we expect to do here? This is especially an issue if you're trying to make a relocatable toolchain tarball, where specifying --gcc-toolchain automatically is difficult.

Would it be possible to use a relative path with --gcc-toolchain then this can be checked in either RISCVToolChain.cpp or GNU.cpp and adjusted accordingly?

The GCC toolchain, when given a relative path, already interprets it relative to the working directory the compiler was invoked from, not the directory the compiler is located in, iirc.

In D91442#2399200, @lenary wrote:

I'm worried about this change - I *think* it doesn't cover the existing behaviour of a baremetal GCC toolchain being installed into the same prefix as clang, and clang automatically picking up that baremetal gcc toolchain. What should we expect to do here? This is especially an issue if you're trying to make a relocatable toolchain tarball, where specifying --gcc-toolchain automatically is difficult.

In D91442#2394501, @jroelofs wrote:

Seems reasonable.

I could see someone wanting to use --gcc-toolchain to point at the baremetal toolchain for their target, but that's unlikely to work out of the box anyway. I'd love to know where the Generic_GCC toolchain is used in practice, since that's always seemed quite fragile to me.

Made the condition consistent in both places where Baremetal toolchain is instantiated as suggested in review.

In D91442#2394513, @jrtc27 wrote:

We use pure LLVM toolchains so improving support for that out of the box is good in my books. However, I do worry this is going to cause friction for a lot of people using LLVM for RISC-V; my understanding is that most use LLVM with a GNU sysroot and binutils, and so this looks like it will break their setups. Is there anything that can be done to automatically detect such cases? What does Arm do here?

In D89040#2344267, @ldionne wrote:

ping.

Updated the comments and summary message as suggested in the review.

In D84708#2320040, @delcypher wrote:

@abidh Can you explain why this is needed? The emulator is set at CMake configure time and its value depends on the target being tested. Why do you want to override this with a value from the lit_config object?

In D88825#2312905, @ldionne wrote: