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

Add some basic guidelines around using C++ standard headers now that our build allows us to #101088

Merged
merged 7 commits into from
Aug 9, 2024

Conversation

jkoritzinsky
Copy link
Member

Just putting some basic guidelines in our docs as a starting point.

@jkoritzinsky jkoritzinsky added documentation Documentation bug or enhancement, does not impact product or test code area-Meta labels Apr 15, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.


### <a name="2.11.2"></a> 2.11.2 Do not use C++ standard exceptions

The exception handling mechanisms in CoreCLR only handle `Exception`-derived types and `PAL_SEHException`. As a result, standard C++ exceptions, derived from `std::exception`, will cause runtime instability and should never be used. This includes using standard collection types with the default `std::allocator<T>` allocator.
Copy link
Member

@jkotas jkotas Apr 16, 2024

Choose a reason for hiding this comment

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

On Linux, we build against ROOTFS generated from Linux version that is out of support. We have convinced ourselves that it is ok given that we are using just prototypes and no actual code from ROOTFS.

Do C++ standard headers come from the ROOTFS? C++ headers do contain actual code (e.g. collection types). If we start including C++ headers with actual code from ROOTFS, our reasoning for why the ROOTFS is ok falls apart.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do use some code from the ROOTFS today. We get some object files linked in from the relevant libc in the ROOTFS. This does give us more exposure to the C++ standard library version from the ROOTFS though, which is worth considering with respect to security issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been investigating this further based on our offline conversations, and I've noticed that tools like createdump are already using C++ headers with the template types. I'd assume that we'd have the same concern there as well as in the hosting layer (which uses C++ standard collections and other template types as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

dotnet/arcade#14728 is the first step towards ensuring we're using maintained/supported C++ standard headers.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

LGTM as long as @jkotas's concerns about building from a ROOTFS are addressed.


### <a name="2.11.2"></a> 2.11.2 Do not use C++ Standard-defined exceptions

The exception handling mechanisms in CoreCLR only handle `Exception`-derived types and `PAL_SEHException`. As a result, standard C++ exceptions, derived from `std::exception`, will cause runtime instability and should never be used. There is one standard C++ exception type the CoreCLR infrastructure supports, `std::bad_alloc`. Since CoreCLR supports `std::bad_alloc`, the standard container allocators, `std::allocator<T>` and the standard C++ containers, can be used as long as only the non-throwing members are used.
Copy link
Member

Choose a reason for hiding this comment

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

as long as only the non-throwing members are used

What is an example of throwing member that cannot be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

std::vector<T>::at() is the first one that comes to mind. I'll add it as an example (and mention to look at cppreference.com or the C++ standard to check the contracts for members).

Copy link
Member

@huoyaoyuan huoyaoyuan May 18, 2024

Choose a reason for hiding this comment

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

Is bad_alloc support something new or existed before? Can STL containers be used more to replace custom containers?

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT May 18, 2024

Choose a reason for hiding this comment

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

I'll add it as an example (and mention to look at cppreference.com or the C++ standard to check the contracts for members).

This is not going to be sufficient. There are many more concerns with these collections and their use in the coreclr codebase. I appreciate we are starting to document this, but the bad_alloc case is the tip of the iceberg here. We have no support in our system's contracts, these have caught innumerable bugs at dev time, so we need a conversation on that. Functions that are marked noexcept and encounter an exception will now result in a C++ run-time's "fail fast" occurring - this would be very new behavior that must be carefully considered across all standard implementations since currently our contracts would find it or it could be caught by some EX_TRY.

My point here is this guidance will need a lot more consideration until we use any C++ collection. Introduction of this kind of use in the .NET 9 release should be avoided or only in the most trivial and appropriate cases. We've a lot to learn in this area and accomodate in the current code base. My recommendation is we approach this cautiously.

Can STL containers be used more to replace custom containers?

This is going to be a long term goal and not something we are likely to accept from community members until the core team develops more guidelines on use. Really appreciate the enthusiasm and look forward to help in this area, but for .NET 9 and likely .NET 10 our use of these collections is going to be narrow and very limited.

@jkoritzinsky
Copy link
Member Author

Circling back to this: Trying to use libc++ with libstdc++ as the ABI doesn't work for Ubuntu 16.04. As a result, I've updated the guidance to recommend against template types for now. We may revisit these rules in .NET 10, but these rules should be a good starting point.

@jkoritzinsky jkoritzinsky requested a review from jkotas August 8, 2024 17:10
@jkoritzinsky jkoritzinsky merged commit 0074d77 into dotnet:main Aug 9, 2024
13 checks passed
@jkoritzinsky jkoritzinsky deleted the stl branch August 9, 2024 05:18
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants