Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC:
#[export]
(dynamically linked crates) #3435base: master
Are you sure you want to change the base?
RFC:
#[export]
(dynamically linked crates) #3435Changes from 1 commit
33968d1
4054f1b
2fb0433
bb2acd3
9b3e6d1
4aa6cd9
61083fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my main question after seeing this example is whether this proposal requires a stable Rust metadata format, or it just works on symbol names.
How is
library::hello
resolved here?Does
library
contain some metadata saying that the crate root module contains a namehello
and it's a function?That means keeping and reading Rust metadata.
Keeping metadata format compatible across rustc versions (by making it stable, or by keeping logic for reading all previous metadata versions) would put a lot of maintenance burden on rustc development, maybe a prohibitive amount of maintenance burden.
Or we just hash the whole
library::hello
path to obtain some string likehello_HASH
and dlsym that string from the dynamic library?In that case hashing the path only seems not to be enough, because it means the argument types and return type are not included into the hash.
Do we need to hash the whole call expression
library::hello()
instead? Even then we don't know the return type to hash.Also, if the hash requires knowing types then resolution of such paths needs to be delayed until type checking, is that right?
I don't like any of these alternatives, TBH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you fully read the RFC? The
library::hello
path in that example is resolved no different than it is today. There is still a crate namedlibrary
with ahello
item. I'm not proposing to somehow enable generate symbol names out of thin air based on just the call expression. The symbol name comes from the definition of the item.A metadata format is perhaps a future possibility, but definitely not part of this RFC. This RFC proposes symbols with a hash based on all information relevant for type safety. That hash does need to be stable, but can start out with strict limitations for only certain kinds of signatures and then slowly extended over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I don't understand how can the dynamic library and the application be built by different versions of rustc ("2. Cases where dynamic library and application can be compiled and shipped separately from each other.").
To resolve paths like we do it today we need to read a lot of various Rust metadata from the dynamic library, but the metadata details typically change between every rustc release and rustc 1.N.0 won't be able to correctly parse metadata generated by rustc 1.M.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the whole RFC, and I have an impression that either I'm missing something big, or it focuses in great detail on secondary issues while skipping on the primary problem with the metadata format incompatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say that we want to dynamically link
library
compiled by rust 1.M.0 withmain
compiled by 1.N.0 (where N > M for concreteness). My understanding is that you would build bothmain
andlibrary
with cargo + rustc 1.N.0 while signaling that thelibrary
dependency isdynamic = true
. This is basically exactly like a statically linked build process except that the code oflibrary
is not emitted and instead symbols using the stable ABI mangling scheme are called instead, i.e.main
would call_Rlibrary5hello_ABCD
or what have you.The previous compile of the library crate by 1.M.0 also produced a function of the name
_Rlibrary5hello_ABCD
because it had the same stable ABI hash, so when these two are linked together (as one would do for an FFI binding) everything works out. The metadata itself is not shared.There are some future work discussions about being able to skip parts of
library
while buildingmain
on 1.N.0 (since most of that work would be discarded anyway) but it doesn't seem to be part of the RFC as written.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, the "Building Dynamic Dependencies" paragraph talks about this, although in a way that is obscured by potential future optimizations.
So, we
cargo build
on that dynamic library we are now basically doingcargo check
on it (with rustc 1.N.0) to get its rmeta.And the benefits we get (compared to just building the dynamic library with 1.N.0) are
All this sounds more or less reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we only need the metadata and not the function bodies, we could skip some things. Once cargo has a separation between public and private dependencies, we can skip private dependencies entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need private dependencies if they are proc macros. Proc macros may expand to
#[export]
attributes or items with#[export]
attributes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think static/dynamic is the correct distinction to make here. I can see a use case for statically linked dependencies that require a stable ABI: proprietary libraries. This is quite common on Windows with C/C++ libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Perhaps that means we should use slightly different names, but I don't think that changes anything else substantially about the RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider throwing in a quick mention that removing
#[export]
is considered a major breaking change since it affects the ABI. I think this is pretty clear, but will let me have an official document I can cite incargo-semver-checks
when we implement a semver-check for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How ABI stability relates to API version is an interesting question. I'll have to think about that a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like a sounding board to bounce ideas off of, give me a ping!
I started the
cargo-semver-checks
project so I've been trying to answer the same question coming from the opposite direction — e.g. we already treat removing#[repr(C)]
as a major breaking change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does Cargo handle the case where a crate is used as a dynamic dependency in one place in the crate graph, but as a normal dependency in another place? I presume in that case it is still compiled as a normal dependency, but the crate declaring it as a dynamic dependency is still restricted to only using exported items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I'd just defer that to the 'unresolved questions' section of the tracking issue. I think I agree with your expectation, but I don't feel strongly about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one issue we'll have with compiling a crate once for making a
.so
file and again for making a program that links against the.so
is that compiling the same code multiple times isn't guaranteed to produce the same output, an extreme example is a proc macro designed to produce random numbers, which then end up as constants in the generated code and/or symbol names.e.g.: https://crates.io/crates/const-random which is a dependency of the very widely used
hashbrown
crateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this RFC is that you do not need the exact same crate for creating the .so and for linking to it. (So you can swap compatible versions, compile it with a different compiler or with different settings, etc.) You only need something that is ABI compatible, and having a mismatch does not result in memory unsafety thanks to all relevant safety information being hashed into the symbols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's one point where this does not work: when safety depends on functional properties of upstream code. Imagine crate A exporting a
fn make_int() -> i32
. Create B re-exports that function. Crate C does something likeThis is perfectly sound as long as
B
promises that its function always behaves like that ofA
. Even if an updatedA
changes the integer returned, things will keep working.But if we are in a dynamic linking situation, can't it happen that we get one version of the function via
B
(since maybe it got inlined or whatever) but a different version viaA
(since maybe we are dynamically linking that one)? Basically exactly the reason we need hashes for types with private fields?OTOH it seems like there are other issues in that situation (e.g. a
static mut
inA
that would suddenly behave differently because now there are 2 copies of it), so I might also be misunderstanding what exactly is going on during dynamic linking here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung Did you mean to call
make_int()
there, or compare the function pointers? Obviously unlessmake_int()
is a pure function we would not necessarily be allowed to assume it always returns the same value, even if it wasA::make_int() != A::make_int()
in the comparison.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I was assuming a promise of
A
that the function is pure.(It's common for unsafe code to depend on functional properties of safe upstream code like that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would imagine that the default behavior of Cargo in this case would be to build the full crate, but emit it as a separate
.so
file. Building just the current crate without the dynamic dependency would be done only if the metadata is available without the source code, but this is future work and not part of the initial plan.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends a bit on the use case what makes most sense as the default. E.g. I wouldn't expect cargo to start building
libjsonparser.so
if we expect that library file to already exist on the target system. (Just like how we don't attempt to buildlibc.so
orkernel32.dll
or similar.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that I could see being different for different profiles too. E.g. you may prefer to build from source for
cargo test
so you don't need the correct version installed, but any release builds would want to link the system. It would be nice if there were some way to configure this.I assume the rustc flag will be something like
--extern=dynamic=...
that treats the extern crate like a header file?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will the format of the crate metadata used to lookup symbol names and function signatures be? Will it be some stable binary format or a rust like syntax that users can directly write? The former is faster, but the later is easier to avoid breaking across rustc versions and makes it easier to prevent the user accidentally breaking the ABI by requiring the interface file to be modified for this to happen. Also will dylibs embed the crate metadata as a whole, just a hash to check compatibility or not at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exact hashing algorithm is something to be designed later (hopefully in an incremental way so we can support e.g.
i32
before we support more complicated types).Ideally it should be specified in a way that it can also be generated by other tools that are not rustc.
Dynamic libraries will include these hashes in the symbol names. Including more information can be useful for debugging tools, but is not necessary. (See the "future possibilities" section.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean dynamic linker here, or is this to say that dynamic linking of rust libs would be skipped at initialization, and evaluated lazily by the loader? On that note, I don't see any mention of dynamic loading rust libs anywhere; maybe it's out of scope, but seems like it should be mentioned, if only as future work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I used "loader" as synonym for "dynamic linker" (e.g.
/lib64/ld-linux-x86-64.so.2
or w/e your system uses). The part of the operating system that loads dynamic libraries and resolves symbols when executables are loaded.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they're typically the same file in practice, I just mean to ask whether libraries would be linked/loaded before or after
.init
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be able to handle multiple semver incompatible versions of the same crate. We currently include the
StableCrateId
in the symbol name for this which includes all-Cmetadata
arguments. Cargo uses a separate-Cmetadata
value for every crate version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we can properly support that. If e.g.
libmycrate.so
is separately compiled, we cannot (and should not) make the symbols dependent on the crate id it will have in a dependency tree of whatever binary will use this library later.Perhaps we want to include just the major version in the symbol names, but not everyone uses versions (a cargo concept) or follows semver, nor does the version of the public API need to match the public of the stable ABI.
Maybe we need an crate-global attribute to optionally specify the ABI version (or even just a random hash) for the crate, to allow for symbols with identical paths and signatures of multiple identically named crates to exist at once, although I imagine most use cases would not use e.g. two different versions of
librustls.so
at the same time. (If that's a supported use case, they should probably be calledlibrustls1.so
andlibrustls2.so
, usingrustls1
andrustls2
as the name used in the symbols rather than justrustls
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
StableCrateId
does not depend on any dependencies. It only depends on the crate name, if it is a lib or bin and all-Cmetadata
arguments (in sorted order). The SVH (crate hash) does depend on dependencies. In addition the current cargo impl hashes dependency versions into the-Cmetadata
argument it passes, but that can easily be changed to only pass the major part of the semver version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work, but I'm not convinced the API version and the ABI version should be treated as one and the same. Renaming structs or non-exported pub functions/methods etc. etc. could result in an incompatible API while preserving a compatible ABI. And in the other direction, a change in invariants or private fields could result in an incompatible ABI while preserving a compatible API.
As I mentioned above, it seems a weird situation if a binary somehow ends up depending on two different
librustls.so
, considering they'd both have to have the same file name. (And if those are calledlibrustls1.so
andlibrustls2.so
, then therustls1
andrustls2
names are probably what should be used in the symbol names rather than justrustls
. And that could just be a build option to cargo or a property in Cargo.toml.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These arguments actually bring into question whether there should be a supported ABI version in Cargo as well. Since Cargo already reconciles latest-compatible updates for APIs, why shouldn't it do the same with ABIs too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is builtin type right? For example,
*const Foo
is stable ABI but is opaque-ish if Foo isn't export too. &str is also builtin but doesn't feel like we should commit to a stable ABI for (at least yet). Maybe worth listing the specific types here rather than naming a category?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this work is largely being offloaded to the "crabi" ABI RFC. Probably the best thing to say here would be "FFI-safe type" in the sense of the
improper_ctypes
lint.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that lint is appropriate here, it has the same questions/problems with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this RFC in particular would need to solve that problem though, the problem is pre-existing and this RFC isn't trying to address it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This RFC is prescribing an error, rather than a lint, and seems intended to start with a very small set and expand over time. So it seems to me that it is trying to do something different; it's also a safe interface AFAICT - so that also matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I should clarify this section a bit. The rules I had in mind are a bit stricter than what I wrote down in the RFC. (Matching your expectations.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that dealing with private fields is very complex and probably not something that many people can commit to. I would rather see a system where a type with private fields is treated as an opaque
extern type
(#1861). This effectively means that you can only interact with this type by reference and all operations on it must be done through exported functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that for many use cases, especially once "crabi" supports dyn trait objects. But for types like NonZero or File, we should be able to allow them to be used by value, since the invaraint / private details will likely not change. (We can still pick a new hash if File does change, but "it's just a file descriptor / handle" will be true for the forseeable future.)
I do agree this is a feature that should be less often than opaque dyn-like objects or fully public types, which is why I think it's fine that exporting a type with private fields requires an unsafe attribute, steering people away to safer alternatives for most situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that helps about the private invariant issue -- if the invariants change, it is quite important that this is considered a separate type, even if everything is by-reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a brief mention of tool support here. People aren't typically familiar with the facilities to generate random numbers locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be surprised if rust-analyzer would get some kind of "generate new random number" action for these attributes.
Producing a random number as a suggestion in the diagnostic when leaving the attribute empty (which could be used by cargo fix) seems helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why does it have to be a random number in the first place? As I mentioned before, this is terrible for code review - if the point is to avoid some kind of collision then it is susceptible to spoofing. A simple incrementing number or string should be sufficient for ABI versioning purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not sufficient. It needs to be globally unique. Otherwise you'd need to (unsafely!) assume that the same (crate) name always refers to the exact same codebase with linear versioning. If a crate is forked and both forks introduce a different new safety invariant on private fields, it would go wrong if both just increment their number, resulting in the same number being used for two ABI-incompatible but identically named types.
In the Rust ecosystem there is no expectation that crate/package names are globally unique. (There exists a lot of Rust code outside of crates.io.) There is no expectation to put e.g. a domain name in the package/crate name to make them unique, like in some other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also why I suggested a string instead of a number, it's a lot easier to put necessary disambiguating information in a string compared to a number. But I don't see how this changes the equation really, since it's unsafe either way. The rule is not "it must be globally unique", it is "this safety invariant must exactly match the safety invariant on any other ABI compatible types with the same
unsafe_stable_abi
which are linked with this crate" which is a much more manageable auditing task.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that "this crate" is meaningless. Outside crates.io, crate names aren't unique.
We could easily allow a string literal instead of a hash, such that you can write "the first field must be a valid file descriptor and the second field must be a prime number" (and hash that) rather than specifying a random token like b58d7aabb705df024fa578d9a0e20a7c, but I'm not sure if that's much better.
Using GUIDs to uniquely identify something seems pretty standard for many forms of identification related to dynamic linking/FFI in various languages.
Another option would be to specify one random hash/guid for the whole crate at the top level, and then use only simple incremental numbers for the types in the crate. But that has a higher chance of resulting in issues when working on multiple versions/branches, if two branches both bump a number up to the same new number with incompatible safety invariants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This crate" is meaningful, I mean this particular linker object that is linked with other things. This constraint is not something the crate author can check, and the (non)uniqueness of crate names doesn't really matter here. It is the author of the final binary who has the responsibility to not link multiple copies of a crate with the same unsafe crate ABI and different actual semantics, and we can provide some assistance in making it difficult to accidentally break this rule, but we can't check this condition, and using hashes only obscures the problem, it doesn't make it any easier to solve.
Really? A string with the literal safety comment would be much easier to verify than a random string of digits (of unknown provenance). And since it's free-form, crates can set up their own policies on how to use the field, i.e. use sequential versioning, hierarchical versions, short identifiers, or full safety comments.
How do I know that your UID is actually unique? Maybe the original programmer knows because they used some "generate random number" utility themselves but what about everyone else?
Hand validation of GUIDs isn't something anyone does though, and anything which is
unsafe
basically has a "please validate me" sign on it. Hashing crate names in mangled names is fine since the generation and uniqueness of these IDs is handled by rustc itself, no one has to look at them directly. Putting them in the source code is a completely different ballgame.Certainly you could mix a crate hash into the type's hash, although that would prevent use cases where two types are deliberately sharing the same unsafe crate ABI despite being defined in different crates (because they are actually supposed to be ABI compatible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this RFC might want to talk about why this is the right choice where we previously made a different one in the move from legacy to v0 (which moved away from a hash, at least mostly). I think given the use case here this may be better, but the discussion seems worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like a good idea to me to combine both of them --
v0
mangling so debuggers and similar can get the full source symbol name including generic arguments, and the hash to detect ABI breaks (ABI breaks won't necessarily change thev0
mangling, such as reordering struct fields)