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

Set up default allocator_library and global_allocator_library #2172

Merged
merged 8 commits into from
Sep 29, 2023

Conversation

illicitonion
Copy link
Collaborator

This makes C++ depending on Rust Just Work out of the box, while still giving people
hooks to customise these if they need.

In particular, this means we can centrally manage and update our default
definition of these symbols, rather than require people to vendor them
and keep their copies up to date as new versions of Rust come out.

We provide this as a best-effort tool which we hope will be useful to
people, and will attempt to maintain the C++ code included here, but
make not guarantees about its universality or correctness. If you need
more specific behaviours, you should set your own allocator_library
and global_allocator_library attributes on your toolchains.

This makes things Just Work out of the box, while still giving people
hooks to customise these if they need.

In particular, this means we can centrally manage and update our default
definition of these symbols, rather than require people to vendor them
and keep their copies up to date as new versions of Rust come out.

We provide this as a best-effort tool which we hope will be useful to
people, and will attempt to maintain the C++ code included here, but
make not guarantees about its universality or correctness. If you need
more specific behaviours, you should set your own `allocator_library`
and `global_allocator_library` attributes on your toolchains.
@illicitonion
Copy link
Collaborator Author

cc @keith as the original author of this code (thanks so much for putting it together and upstreaming it, by the way!), @UebelAndre as the most recent person to edit it.

I don't know if there are major hazards from doing this, and would be interested to hear any - the two obvious ones that jump out are that this interface may not be stable (particularly across Rust versions) so we may struggle to maintain it, and that if people are using custom allocators it may not work (but they can suppy their own implementations in this case).

(Also, supporting Windows seems hard, and I'm very happy to just punt on it for now)

@krasimirgg
Copy link
Collaborator

If I remember, previously the main point against that was that folks may need to have slightly different definitions based on their concrete combination of platform, C/C++ and rust available. I can see a lot of potential of having some defaults that "just work" for most users most of the time, however. This seems to fall just in this grey area.
If there is good way to effectively communicate the intent that this feature is best effort only and folks may need to provide their own definitions if stuff doesn't quite work in their environments, I'd be very enthusiastic about this.
Curious also about @scentini's thoughts.

@scentini
Copy link
Collaborator

IIRC the reason why we didn't do this back when we initially added support for invoking the linker directly was because it is not officially supported. I'm fine with this PR, with one nit - I believe in the current form we always pass the allocator_library to the linking action (expecting that it is None when we don't use experimental_use_cc_common_link) (follow code from here), we should probably make sure that we only pass it when experimental_use_cc_common_link is enabled.

@illicitonion
Copy link
Collaborator Author

I believe in the current form we always pass the allocator_library to the linking action (expecting that it is None when we don't use experimental_use_cc_common_link) (follow code from here), we should probably make sure that we only pass it when experimental_use_cc_common_link is enabled.

I'm not sure about this... AFAICT, experimental_use_cc_common_link only controls how we link rust binaries, and the allocator library is only used when we link cc binaries... I don't know this code at all, so I could be very wrong, but that's my read from poking around a bit?

@scentini
Copy link
Collaborator

The allocator library is used both with experimental_use_cc_common_link and when cc_binary is driving the linking. But I got confused and forgot about the latter :)

@illicitonion illicitonion merged commit aa4b3a8 into bazelbuild:main Sep 29, 2023
3 checks passed
@illicitonion illicitonion deleted the default-allocator-library branch September 29, 2023 13:45
@keith
Copy link
Member

keith commented Oct 24, 2023

thanks for doing this! this is what I was hoping for originally, can't remember why we didn't but there was some conversation about that in #1238 (comment)

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.

4 participants