Sorry for the delay, I though this was merged already. LGTM.

I'm curious what is your system configuration where this patch actually allows for detection of devtoolset? I noticed that if clang and gcc are both installed to /usr/, then driver will pick the gcc in /usr/ over the one in /opt/rh/.../

This

Updated documentation in BitCodeFormat.rst.

@tejohnson Are you OK with this change?

Update comments and documentation.

Add documentation about the change.

This was committed to the release/14.x branch: 2e7e14177186dc417a49dbf88493dabde0572e22

In D125165#3504252, @tra wrote:

LGTM in principle.

Given that we're introducing a new tool dependency we may want to get a stamp from someone dealing with build and release.
@tstellar -- do we need to change anything else for the new binary to ship with clang releases?

This looks OK to me.

@jyknight You commented here that -fembed-bitcode was not intended for LTO, but the llvm documentation says the .llvmbc section is useful for LTO. Do you have any more background on the history of the .llvmbc section and do you think the gold plugin should try to use it for LTO?

In D124153#3479388, @thieta wrote:

With this I suggest we should go ahead and:

• Improve documentation - Please help out in the review posted above.
• Make omitting CMAKE_BUILD_TYPE an error and link to the improved documentation.

This seems like a good compromise +1.

In D124539#3479378, @thieta wrote:

I think this is fine. But I wonder if we should split out the version into a separate file that can be easier to parse and so we don't have to edit the version in several different places. In my last job we usually had a single VERSION file in the root that we read from CMake and all other places that needed the current version.

I think for now what I'll do for now is just update D124405 to list libunwind as a required sub-directory for lld stand-alone builds. This will get me unblocked and make it possible to set up the CI.

@lebedev.ri are you still working on this or should someone else commandeer the revision?

@int3 @MaskRay The kind of stand-alone builds we are trying to support are documented here: D123968

In D124153#3471155, @thieta wrote:

One thing we all seem to agree on is that documentation could be better - so I took a stab at that here - I think it's connected to this discussion, but I don't think it's the only fix, please see the doc suggestions here: https://reviews.llvm.org/D124367

I am currently leaning towards:

• Improve documentation
• Make omitting CMAKE_BUILD_TYPE a error. I am also fine with changing it to Release, but I don't have a strong opinion either way. I do think keeping Debug is worse than both options. But it can be made better by point 3 here:

In D124315#3469495, @smeenai wrote:

In D124315#3469458, @tstellar wrote:

In D124315#3469449, @smeenai wrote:

To be clear, this is to support a build scenario where you only have the source tree under lld available?

Yes, that's correct.

Ah, I'd missed that part of the definition in https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291. Thanks for confirming.

The duplication here is really unfortunate, especially cos the burden of keeping the copied header up-to-date will fall on non-standalone build users. On the other hand, this header hasn't seen actual (non-license) update since 2016, so I imagine it'll remain pretty stable going forward.

In D124315#3469449, @smeenai wrote:

To be clear, this is to support a build scenario where you only have the source tree under lld available?

Remove debug message.

In D124153#3468477, @erichkeane wrote:

If you want a 'right' default, it has to be 'the one that most users of cmake WANT'. I posit that is 'default' or 'release with debug info + asserts', since MOST direct users of our cmake scripts are going to be developers. I'd find 'no default' to be a vast improvement over 'give me the wrong one'.

In D124153#3468363, @tstellar wrote:

I think we pretty much agree on everything here, except I do think build mode will actually help reduce this problem a lot.

In D124153#3468341, @aaron.ballman wrote:

In D124153#3468313, @tstellar wrote:

@aaron.ballman There's some people that can't build in debug mode at all because there machine is not powerful enough. For me, this is enough to disqualify it from being the default, because it's creating a barrier of entry for new users/developers.

The same is true for making release builds; you can OOM a machine trivially when linking. To me, the barrier to entry isn't that the default build mode is wrong, it's that we've not done a good job of making our project buildable without a build farm or super computer. I think changing the default build mode to release is a bit like using a teaspoon to bail out a leaking boat; it's incrementally better than nothing, but it's really not addressing the problem or likely to make a significant difference.

In D124153#3468300, @mehdi_amini wrote:

We have some pieces of data though: the number of people who complain about the build filling up their disk and the link taking forever (or OOM). On the other hand the people who "know" are already setting up an explicit value...
That said I tend to think that the audience is heterogeneous enough that I agree it seems hard to make sure we "pick a default that's "right" " in the absolute (hence my proposal to not pick any).

@aaron.ballman There's some people that can't build in debug mode at all because there machine is not powerful enough. For me, this is enough to disqualify it from being the default, because it's creating a barrier of entry for new users/developers.

In D124153#3467965, @aaron.ballman wrote:

In D124153#3467906, @tstellar wrote:

The target audience for the default configuration should be people who are new to the project, it could be users or it could be developers. It also could be people with very little experience programming or building from code from source. We want to keep the barrier for entry as low as possible for this group of people. We can have the best documentation or have verbose error messages to help guide them in the right direction, but the best way to communicate to new users about how to build the project is to have a sensible default configuration.

I agree with almost every point, right up until "have a sensible default configuration"; I don't think it's possible to have a default configuration that is sensible to both users and developers. Well, I take that back. I think it *is* possible, but I think we would have to invest significant effort in improving the LLVM compilation performance issues so that you don't need a super computer to build the project. My understanding is *that* is what's most off-putting to new folks, whether they're users or developers, and that's somewhat (but not entirely) orthogonal to the default build mode.

The target audience for the default configuration should be people who are new to the project, it could be users or it could be developers. It also could be people with very little experience programming or building from code from source. We want to keep the barrier for entry as low as possible for this group of people. We can have the best documentation or have verbose error messages to help guide them in the right direction, but the best way to communicate to new users about how to build the project is to have a sensible default configuration.

In D124153#3466495, @mehdi_amini wrote:

In D124153#3466424, @tstellar wrote:

I disagree about erroring out when CMAKE_BUILD_TYPE is undefined. We want to make the build easy for new users, so I think it's important that the default build (cmake with no arguments) just works.

Why isn’t it easy to copy/paste one option clearly presented by the cmake invocation? That seems like a good user experience to me: there is a fairly simple choice to be made and we expose it concisely and directly, it’s not like there is an obscure error and the user needs to read a doc…
It’s not like every command line tool works without mandatory options.

I disagree about erroring out when CMAKE_BUILD_TYPE is undefined. We want to make the build easy for new users, so I think it's important that the default build (cmake with no arguments) just works.

Address review comments.

LGTM.

In D123850#3453750, @cor3ntin wrote:

@tstellar I wasn't sure what the procedure was for backporting fixes, so I made a PR. I think your suggestion of keeping the old signature works well.
This also modifies the serialized AST, is that an issue for backports? (If it is, I'm not sure i see a way to move forward with this!)

I will try to be more specific:

@lebedev.ri This commit message is not appropriate. Please change it in before committing to the repo and also update this review with something else.

Ok, so maybe my concerns about testing overhead are not that legitimate. That's just something I've heard mentioned in the past. However, I still think this should be fixed in buildkite. It's seems much more sustainable to just bump the timeout in buildkite rather than having to go through the code and split tests every time they get too long. It sounds like the maintainers have been contacted about this, but haven't responded?

Is there some reason we can't use the c++11 regex library for this?

Hi, I think this change caused the llvm-clang-win-x-aarch64-release builder to fail. I'm seeing the following error in the logs which seems to be related to this change:

Can't we add a timeout exception for this test in buildkite? Splitting the file in two actually increases the runtime, due to the overhead of setting up each test.

This looks good, thank you.

Can you elaborate more on the problem this is solving? Also, what are the user visible changes? Will check-all and check-clang run the same set of tests as before?

@aganea I think it's OK to land D123185 and then backport both patches.

In D77945#3424750, @mstorsjo wrote:

In D77945#3423692, @tstellar wrote:

@mstorsjo Ok, so is this patch no longer needed?

Indeed, this one was superseded by D109493, so this one can be closed.

@mstorsjo Ok, so is this patch no longer needed?

Why is i686-pc-windows-msvc is correct triple?

LGTM.

Reference correct repository for the workflow definitions.

In D122487#3408425, @rs wrote:

@tstellar thanks for the review. I can't really think of any other way to test this. I'll wait till Monday if no one else has suggested anything else then I'll commit it. Thanks again.

The reason I used a minimal check string is because if the warning text changes at all, then the test becomes useless (this is the downside of using -NOT). I wonder if there is another way to test this that might be robust? If not, then I think this change is fine.

In D122082#3397728, @beanz wrote:

In D122082#3395747, @nikic wrote:

@xbolva00 Don't think the board has any relation to this, but I do agree that having some central documentation would be great. While many of the ongoing migration efforts are well-documented by themselves, there's currently no central place that collects them. I've created https://reviews.llvm.org/D122122 as a starting point, feedback welcome.

@nikic, even that document doesn't say support for opaque pointers is a "hard requirement" for new contributions.

Given that there is no documentation or clear community consensus anywhere that I can find that it should be a hard requirement, and that this code is going into an experimental backend, which (as documented in the support policy: https://llvm.org/docs/SupportPolicy.html) means "you break it, I fix it". Would you revise your "needs revision" feedback?

In D118493#3398808, @JonChesterfield wrote:

It lets applications run with the libomp and libomptarget built with the toolchain. For users, they don't have to be root. For compiler devs, we test our work instead of whatever the distro had installed.

There's no info at that link. What's 'broken rpath'? If they mean runpath, then it's really on fedora to have Wl,--rpath set rpath instead of runpath. Setting the default in the linker to the one your own build tools reject will break everything, not just llvm openmp.

Edit: https://fedoraproject.org/wiki/Changes/Broken_RPATH_will_fail_rpmbuild
Doesn't mean runpath. There's a list of strings Fedora (IBM?) have decided they don't like, named 'broken' or 'invalid', and declared shall not be present. Page doesn't go into the rpath/runpath distinction, maybe they mean for the string restrictions to apply to both.

@XiaodongLoong You may be interested in the toolchain integration test suite which has multi-project tests like this.

Fedora recently banned RUNPATH, so packages built with clang and -fopenmp are rejected by the build system with this change. Can you elaborate more about the intended use case that this change will fix? Is it just meant for when libomp.so is installed outside of /usr/lib64 ?

Would it be easier just to package the cmake directory in a separate tarball?

@xbolva00 I've removed your comment due to inappropriate language and tone. Please find a more constructive way to communicate your point.