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:

In D86920#2271948, @ldionne wrote:

Please take a look at libcxx/test/configs/libcxx-trunk-shared.cfg.in and LIBCXX_TEST_CONFIG in CMake for how to customize exactly how you're running the test suite. This is a lot more powerful and simple than stringing options through CMake!

Ping.

In D86540#2236228, @gargaroff wrote:

We require this patch for our downstream baremetal target to be able to run the test suites of both libraries on our target.

I'm not sure whether the check in libcxxabi was simply outdated or if we will require some type of check there.

I also maintain a downstream baremetal target and have a similar local change to side step this check. Although I did not remove it entirely as I was not sure what it does and instead used NOT LIBCXXABI_BAREMETAL. Anyways, It will be nice to have this fixed upstream.

Handled review comments.

Moved test to linkerscript/define.test as advised.

I have removed the .eh_frame. I have looked a bit more into it. It seems that riscv target does create relocations in .gcc_except_table. The following commit gives some background on its encoding.
https://reviews.llvm.org/rGab009a602e96b238000d9e20e5c54b078d08aad3

In D83244#2133669, @MaskRay wrote:

The .eh_frame test case is invalid. LLD handles .eh_frame input sections differently. It parses .eh_frame and deduplicates them. See eh-frame-merge.s, an input .eh_frame referencing a non-prevailing COMDAT group is dropped (EhFrameSection::isFdeLive)

Do you have a realistic case where LLD erroneously errors? If so, can you get a minimal reproduce, use LLD_REPRODUCE=/tmp/rep.tar or -Wl,--reproduce=/tmp/rep.tar to get a reproduce file and upload it somewhere?

Looks ok but I would like a testcase to go with the change.

The lldb-mi bits look ok to me.

This check used to be there above the loop and was removed when you reported that changes in pointers are not being tracked in
http://lists.llvm.org/pipermail/lldb-dev/2017-May/012428.html