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

Support target_os = "none" #70

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Conversation

nspin
Copy link
Contributor

@nspin nspin commented Jan 24, 2024

Alternative take on #66.

Adds a feature called force-init-array-section to force __do_submit! to assign static __CTOR to the .init_array section, regardless of the platform. This enables support for arbitrary platforms that support .init_array constructors.

Unlike #66, this is an intrinsic feature of the inventory crate.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! I was thinking of suggesting something along these lines in #66.

I don't know that a feature is appropriate for this. Features are required to be additive i.e. some crate not enabling the feature must be indifferent to any other crate enabling the feature.

Force-init-array-section needs to be a choice made by the top-level build, rather than any particular dependency edge on inventory. Can you do this using --cfg=force_init_array_section?

src/lib.rs Outdated
Comment on lines 484 to 487
any(target_os = "macos", target_os = "ios"),
link_section = "__DATA,__mod_init_func"
)]
#[cfg_attr(windows, link_section = ".CRT$XCU")]
Copy link
Owner

Choose a reason for hiding this comment

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

What is the intended behavior if force-init-array-section is enabled when the target OS is one of the recognized ones such as Windows or macOS? Is it better to only apply to target_os = "none"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the behavior of this first attempt isn't quite what we're after.

I just pushed a new attempt where the feature is renamed to default-to-init-array-section. The new behavior is that the feature just causes the crate to default to using .init_array.

This approach has the advantage of not deviating from the current behavior on supported platforms, while also not being limited to just those unsupported platforms where target_os = "none".

Copy link
Owner

Choose a reason for hiding this comment

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

I am confused / interested to hear more about the use case you envision for default-to-init-array-section on target_os values other than "none". Does there exist any target-os on which .init_array sometimes works and sometimes not, such that a feature to choose whether .init_array is used makes sense?

If not, the person with an unsupported target-os should just make a PR adding the new target_os to inventory, rather than working around with a feature that modifies default behavior.

If yes, this PR doesn't seem any better than adding the target_os to inventory anyway to make it unconditionally use .init_array, because the case where .init_array doesn't work is not handled either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment of yours has made me realize that perhaps what I should really be proposing here is just that target_os = "none" be added to the link_section = ".init_array" list.

In the seL4 userspace case, because seL4 is a microkernel with minimal features, an OS is implemented using userspace components. So, these userspace components run at a higher-level than baremetal, but are really target_os = "none". Folks building sophisticated systems compose a bunch of these low-level components to eventually provide a higher-level environment for other userspace components which have the luxury of target_os = "my-special-sel4-based-os".

Before looking at the Clang source, I would have considered target_os = "none" implying link_section = ".init_array" to be too much of a leap, but, as far as I know, target_os = "none" will basically always mean non-legacy ELF (rather than MACH-O or COFF), which, according to Clang's policy (which I'm viewing as informative rather than informative), means .init_array.

Does there exist any target-os on which .init_array sometimes works and sometimes not, such that a feature to choose whether .init_array is used makes sense?

To answer your question, I think not. So yes, it would make sense to limit the scope of this PR to target_os = "none".

@nspin nspin force-pushed the pr/force-section-feature branch from bc8bb7a to df8cd24 Compare January 26, 2024 07:49
@nspin
Copy link
Contributor Author

nspin commented Jan 26, 2024

Force-init-array-section needs to be a choice made by the top-level build, rather than any particular dependency edge on inventory.

I agree.

I've taken a look at how Clang chooses which section to assign constructors to [1]. Predictably, Clang makes its decision based on information about the target (mostly based on the target object format, and notably not based on the target OS), that a crate like inventory does not have structured access to. In the absence of a probing hack in this crate's build.rs, I don't see a way for this crate itself to choose a section for constructors in the same way Clang does.

I don't know that a feature is appropriate for this. Features are required to be additive i.e. some crate not enabling the feature must be indifferent to any other crate enabling the feature.

This certainly wouldn't be the most elegant use of Cargo features.

My new attempt (df8cd24, just force pushed) goes some ways towards addressing your point about the fact that the feature should be additive, perhaps even resolving it. As mentioned in my comment in the review thread above, in this new attempt, the default-to-init-array-section feature just causes unsupported platforms to fall through to #[link_section = ".init_array"] rather than nothing.

However, regardless of the additivity question, another issue with implementing this option as a feature is that it is not a natural fit for cases where a crate which is not necessarily aware of its top-level purpose, target platform, or link-time environment depends on the inventory crate. The expectation would be that a transitive dependent that does have that context would depend on the correct version of inventory crate just to enable that feature.

While this isn't ideal, it may be the best option, provided that it's deemed just a usability issue rather than an appropriate usage of features issue.

Can you do this using --cfg=force_init_array_section

The top-level build could inject cfg(default_to_init_array_section) or similar into the inventory crate either by declaring a --cfg= rustc flag for the entire build or using the unstable profile rustflags Cargo feature [2]. This would work, but would have the consequence of cluttering the top-level build.

For a concrete example of what I mean by cluttering the top-level build, I'll use my use-case for the inventory crate: userspace for the seL4 microkernel [3]. Consider the case where a crate in that repo, which is meant to be used by downstream projects constructing userspace components for seL4, depends on the inventory crate. Such a crate would know it will be linked for seL4 userspace, and could specify the default-to-init-array-section feature. If we used the --cfg= approach, then every downstream top-level build transitively depending on this hypothetical crate would need to worry about that --cfg= rustc flag in its top-level build.

[1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
[2] rust-lang/cargo#10014 (comment)
[3] https://github.com/seL4/rust-sel4

@dtolnay
Copy link
Owner

dtolnay commented Jan 26, 2024

Thank you for the explanation of the seL4 userspace component use case! I see the motivation for this approach.

I looked through https://docs.rust-embedded.org/embedonomicon/custom-target.html + https://doc.rust-lang.org/1.75.0/nightly-rustc/rustc_target/spec/struct.TargetOptions.html and discovered one other option that could be worth evaluating:

families: Cow<'static, [Cow<'static, str>]>

Values of the target_family cfg set for this target.

Common options are: “unix”, “windows”. Defaults to no families.

See https://doc.rust-lang.org/reference/conditional-compilation.html#target_family.

Something like "families": ["constructors_in_init_array"] that would appear in https://github.com/seL4/rust-sel4/blob/main/support/targets/x86_64-sel4.json, and inventory would look for it as target_family = "constructors_in_init_array". Does that sound like it fits the intended use of target family? "a more generic description of a target, such as the family of the operating systems or architectures that the target generally falls into. Any number of target_family key-value pairs can be set."

@nspin
Copy link
Contributor Author

nspin commented Jan 26, 2024

Reading that Clang source file warmed me up to another approach, which is to just default to .init_array instead of nothing.

The Clang source [1] just categorizes targets into ELF|MACH-O|COFF. I think it is reasonable to think that this crate could use cfg to catch all MACH-O|COFF cases, leaving only ELF cases. According to Clang, ELF targets ought to use .init_array, except for a few legacy targets which still use .ctors (e.g. NetBSD, which, by the way, may be a separate issue to raise for this crate) [2].

So, concretely, I'm proposing:

#[cfg_attr(windows, link_section = ".CRT$XCU")]
#[cfg_attr(any(target_os = "macos", target_os = "ios"), link_section = "__DATA,__mod_init_func")]
#[cfg_attr(target_os = "netbsd", link_section = ".ctors")]
#[cfg_attr(not(any(windows, target_os = "macos", target_os = "ios", target_os = "netbsd")), link_section = ".init_array")]
static __CTOR: unsafe extern "C" fn() = __ctor;

[1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
[2] rust-lang/rust@0e7d5be#diff-d27e450f6e879166a979a4d55eef118fb350f966a1669709e9a3420fed5fa326

@nspin
Copy link
Contributor Author

nspin commented Jan 26, 2024

So, concretely, I'm proposing:

Or, better, as mentioned in #70 (comment), just adding target_os = "none" to the link_section = ".init_array" list.

@dtolnay
Copy link
Owner

dtolnay commented Jan 26, 2024

👍 I am on board with either or both of:

  1. Treating target_os = "none" as non-legacy ELF.
  2. Recognizing some particular target_family. I made some attempts in GitHub's code search and failed to find a single example of a Rust custom target setting "families", so I am not sure what conventions exist for naming them. The name in my previous comment isn't great; possibly target_family = "elf" or "exe-elf".

@nspin
Copy link
Contributor Author

nspin commented Jan 26, 2024

Something like "families": ["constructors_in_init_array"]...

Yeah, I think this the direction of an ideal solution; crates having structured access to the target information required to make a robust and complete decision about which section to assign these constructors to. However, because rustc doesn't really support or deal with .init_array-style constructors, it makes sense that such information wouldn't be formally baked into the rustc target spec.

Even without this information being baked into the rustc target spec, the target spec still feels like a nice way to convey less formal target information to crates. I'm thinking specifically about how using the JSON target spec would be much more convenient than having to include some kind of --cfg= directive with every top-level build.

The target_family field does seem like it makes the most sense out of the ones available now [1]. However, in line with your note, the only cases of its use that I'm aware of are in https://github.com/rust-lang/rust, and the only values are unix, windows, and wasm. So, while I hope that the target spec format is extended to enable something like this, the target_family field doesn't seem like a great fit.

[1] https://doc.rust-lang.org/beta/nightly-rustc/rustc_target/spec/struct.TargetOptions.html

@nspin
Copy link
Contributor Author

nspin commented Jan 26, 2024

  1. Treating target_os = "none" as non-legacy ELF.

To me, this feels like a safer approach for now. I'll modify this PR to implement it, but happy to explore option #2 further and switch to that one if you decide you prefer it.

@nspin nspin force-pushed the pr/force-section-feature branch from df8cd24 to b273ebc Compare January 26, 2024 09:45
@dtolnay
Copy link
Owner

dtolnay commented Jan 26, 2024

https://doc.rust-lang.org/1.75.0/reference/conditional-compilation.html#target_family defines target_family as "Key-value option providing a more generic description of a target, such as the family of the operating systems or architectures that the target generally falls into".

It isn't a stretch to extrapolate this to family of the operating systems or architectures or executable formats.

The existing definition already indicates that the use of this field is open-ended, with OS and architecture family just being 2 example use cases, and the fact that only unix windows wasm are used upstream does not make those the only legal values. It sounds like custom targets are allowed to provide an arbitrary string value.

I will consider sending a compiler PR or MCP to fill in target_family = "elf"/"mach-o"/"coff" into the builtin target specs. Or, you should feel free to do so if this would be aligned with simplifying other PRs like this one you need to push to other crates.

@dtolnay dtolnay changed the title Add feature to force assignment to the .init_array section Support target_os = "none" Jan 26, 2024
@dtolnay
Copy link
Owner

dtolnay commented Jan 26, 2024

Similar PR in linkme, also treating target_os = "none" as ELF: dtolnay/linkme#10

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thank you!

I appreciated your expertise and research, patient explanations, and thoughtful suggestions.

@dtolnay dtolnay merged commit 7492bc8 into dtolnay:master Jan 26, 2024
11 checks passed
@dtolnay
Copy link
Owner

dtolnay commented Jan 26, 2024

Published in 0.3.15.

@nspin
Copy link
Contributor Author

nspin commented Jan 26, 2024

I will consider sending a compiler PR or MCP to fill in target_family = "elf"/"mach-o"/"coff" into the builtin target specs. Or, you should feel free to do so if this would be aligned with simplifying other PRs like this one you need to push to other crates.

This is a great idea. You're correct, this would simplify lots of similar PRs across the ecosystem. I'll link it here as soon as I get around to it (if I get around to it before you do) to avoid duplicated efforts.

Thank you for this crate, along with the countless others!

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.

2 participants