dtolnay / inventory

Typed distributed plugin registration
Apache License 2.0
948 stars 43 forks source link

Support `target_os = "none"` #70

Closed nspin closed 5 months ago

nspin commented 5 months ago

Alternative take on https://github.com/dtolnay/inventory/pull/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 https://github.com/dtolnay/inventory/pull/66, this is an intrinsic feature of the inventory crate.

nspin commented 5 months ago

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 (df8cd2484a348d2e5c331bf7e6d9a887f7dcee0a, 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] https://github.com/rust-lang/cargo/issues/10014#issuecomment-1435980653 [3] https://github.com/seL4/rust-sel4

dtolnay commented 5 months ago

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 targetfamily key-value pairs can be set."

nspin commented 5 months ago

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] https://github.com/rust-lang/rust/commit/0e7d5be4b808bd7ccb1d45aa8f7e5026a15aee93#diff-d27e450f6e879166a979a4d55eef118fb350f966a1669709e9a3420fed5fa326

nspin commented 5 months ago

So, concretely, I'm proposing:

Or, better, as mentioned in https://github.com/dtolnay/inventory/pull/70#discussion_r1467395607, just adding target_os = "none" to the link_section = ".init_array" list.

dtolnay commented 5 months ago

:+1: 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 commented 5 months ago

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 commented 5 months ago
  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.

dtolnay commented 5 months ago

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 commented 5 months ago

Similar PR in linkme, also treating target_os = "none" as ELF: https://github.com/dtolnay/linkme/pull/10

dtolnay commented 5 months ago

Published in 0.3.15.

nspin commented 5 months ago

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!