Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(llvm-toolchain): support c++20 with llvm-14 and libc++ [IO-113] #131

Merged
merged 30 commits into from
Apr 2, 2024

Conversation

jungleraptor
Copy link
Contributor

@jungleraptor jungleraptor commented Mar 25, 2024

Changes necessary to enable building C++17 and C++20 code with llvm-14.

Background

The main challenge in implementing this is that the version of libc++ that ships with llvm-14 has several issues when it comes to building c++17 and c++20 code:

  • memory_resource implementation is still in the experimental namespace
  • There is a bug that caused a valid language feature to be deprecated when using c++20 (std::allocator<void> specialization)
  • libc++ is more aggressive in general about removing deprecated language features, necessitating building with -D_LIBCPP_ENABLE_CXX20_REMOVED_FEATURES if your code is still using them.

Design

The first major point is that libstdc++ suffers from none of these issues so I'm electing to make that the default for linux. On mac we need to support libc++, and we would still like to build/test with this configuration on linux, so most of these changes are in support of that.

I've chosen to use Bazel's features API for toolchain configuration to implement support for C++20.

Effectively what this enables us to do is all the user has to do is specify there target is using c++20:

cc_library(
    name = "cpp20",
    ...
    features = ["c++20"],
)

And underneath the hood, if we are using this specific toolchain, and the user is building with libc++ - then we will automatically add the necessary definitions required to the compile line (only for the target that requires it).

I've taken the opportunity to write a robust set of features for setting language standards and implementation extensions so that we can move toward using these features for our general language standard settings (i.e. swift_cc* macros). This will enable us to remove the c overloads of these functions in future releases.

I've also added a relwdbg feature as this is an oft requested improvement to the build system.

These changes will cause a re-extraction/re-build of the codebase - so it's best to pack as many of them in at once as possible.

This has also required a significant refactor of our toolchain internals since previously we were not modelling the standard library choice as a toolchain feature.

Lastly I've also added the patch to the llvm headers required to fix using std::allocator<void>.

Testing

@jungleraptor jungleraptor changed the title feat: support c++20 with llvm-14 and libc++ feat(llvm-toolchain): support c++20 with llvm-14 and libc++ Mar 27, 2024
@jungleraptor jungleraptor changed the title feat(llvm-toolchain): support c++20 with llvm-14 and libc++ feat(llvm-toolchain): support c++20 with llvm-14 and libc++ [IO-113] Mar 27, 2024
@jungleraptor jungleraptor marked this pull request as ready for review March 27, 2024 18:28
@jungleraptor jungleraptor requested a review from a team as a code owner March 27, 2024 18:28
@jungleraptor jungleraptor requested a review from pcrumley March 27, 2024 18:29
Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is quite large and I do not have enough of a bazel/c++ background to really review it well. I'd encourage you to seek out additional reviewers to check out this code as well.

I just have a few main concerns:

  1. Will there need to be additional training/materials that come along with this PR? e.g. Will it break local builds on dev's computers if they don't do some additional steps. Another thing I think is a big one is we have the toolchain be a feature a user can opt into, we have strong guidance on what toolchains people should use where? Does the Orion team already have enough institutional knowledge so that if a dev added c++20 in a PR to a component that needed to be C++14 for autosar, they would reject it?

  2. Did this PR introduce any changes to the compilation etc if you don't opt into the C++20 feature? Its hard for me to tell with my Bazel knowledge, but i think it is important that the changes to how the file has been restructured does not change the toolchain for people not opting into the new features. Or if it did change the toolchain, do we need to get approval or anything on this.

  3. Finally, will this be an issue with customers running our binaries?

I had a minor question inline about static linking as well.

flag_groups = ([
flag_group(
flags = [
# link statically for now
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the static linking a change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the same build flags we've been using - I left that comment here because whether to link the standard library statically or dynamically should be a toolchain feature - but it's going to be a bit of work to make that possible.

@jungleraptor
Copy link
Contributor Author

jungleraptor commented Mar 27, 2024

This PR is quite large and I do not have enough of a bazel/c++ background to really review it well. I'd encourage you to seek out additional reviewers to check out this code as well.

I just have a few main concerns:

  1. Will there need to be additional training/materials that come along with this PR? e.g. Will it break local builds on dev's computers if they don't do some additional steps. Another thing I think is a big one is we have the toolchain be a feature a user can opt into, we have strong guidance on what toolchains people should use where? Does the Orion team already have enough institutional knowledge so that if a dev added c++20 in a PR to a component that needed to be C++14 for autosar, they would reject it?

From the user perspective the only thing that needs to be communicated is that the proper way to enable c++20 for their targets is to use features = ["c++20"],. Most of the point of how we've set everything up is that to hide the actual complexity required to make that work so that nothing else is required on the users end.

As far as when and where it's allowed to be used - that's up to teams to figure out and how they can enforce it. I would say that there's certainly enough institutional knowledge to avoid misusing this feature. There's nothing stopping us from using C++17 or C++20 today in certain scenarios and people have been pretty diligent only using those where it's appropriate.

  1. Did this PR introduce any changes to the compilation etc if you don't opt into the C++20 feature? Its hard for me to tell with my Bazel knowledge, but i think it is important that the changes to how the file has been restructured does not change the toolchain for people not opting into the new features. Or if it did change the toolchain, do we need to get approval or anything on this.

The actual compile line that gets emitted by bazel is virtually unaffected (with the understanding that we are changing our default standard library implementation on linux).

This is purely a refactor so that we can elegantly handle applying the right build flags when C++20 + libc++ is used.

Basically all this amounts to is that -D_LIBCPP_ENABLE_CXX20_REMOVED_FEATURES shows up on the compile line when we combine libc++ and c++20, and only in those contexts (i.e. the rest of the code is unaffected by these changes).

  1. Finally, will this be an issue with customers running our binaries?

This doesn't have any direct customer impact.

Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to write a detailed response to my concerns. 🚀

@jungleraptor jungleraptor merged commit 9b5d663 into main Apr 2, 2024
2 checks passed
@jungleraptor jungleraptor deleted the isaac/cpp20 branch April 2, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants