Add encoding for f16 and f128
#122106
Add encoding for f16 and f128
#122106
Conversation
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use r? to explicitly pick a reviewer |
rustbot
added
S-waiting-on-review
|
This PR reserves space in the v0 symbol mangling grammar for the new @rfcbot merge Thanks for opening the PR, @tgross35! |
|
Team member @michaelwoerister has proposed to merge this. The next step is review by the rest of the tagged team members:
Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
|
What happens when attempting to demangle symbols that use the new encodings with an older version of the demangling code? Since we have some level of support for v0 mangling in the wild, we should consider how making this change will affect existing tooling. |
That is a good question, the docs don't seem to call out any specific forward compatibility or error handling. It looks like I think we can probably live with the fact that existing demanglers will just error until they can update support. To me this does not feel like a breaking change because it does not change anything that currently demangles correctly. It is probably still reasonable to try to make sure at least known demanglers support these symbols before stabilizing the primitives, I added it to the todo list #116909 (comment). We could also update documentation to reserve a set of characters for future primitives that demanglers should gracefully handle ( |
|
Q: does the |
I think ideally it would be different, leaving |
|
Yes, it's likely that existing demanglers will fail if they encounter something unknown in the grammar. If we had to start from scratch with a new mangling scheme, then forward compatibility would be on my set of requirements. As things are now, I think the best approach is to document new additions early (before they are emitted by a stable compiler) and try to get support merged in external tooling during that still-unstable window. |
|
I somewhat regret reproducing the Itanium primitive encoding, it doesn't scale well, so its only benefit is compactly mentioning a lot of primitives (more relevant to C++ overloads than to Rust). I have nothing against the reservation but I should point out there are alternatives. If today While less pretty, we could also reserve special "namespaces" (using crate name+disambiguator values impossible to arise for real crates), or even give new primitives full identities in libcore using lang items. |
Is your position neutral here, or would you prefer investigating the above at this point? I do think there is some nice consistency in saying that all @wesleywiser were the above answers reasonable enough, or did you have any further concerns? For what it is worth, one demangler has already picked up the additions #116909 (comment). |
|
It seems like giving these primitives full identities in libcore is pretty cheap (we'll want methods on them anyway) and presumably means that there's no 3-5 year period while tooling starts to support demangling symbols with them present? That feels like a much better option to me, the only thing we're gaining by doing the current reservation approach seems to be a bit of efficiency in symbol length, right? That seems like a pretty minor win (symbols are already pretty long). I'm still constantly on systems that don't demangle baseline v0 out of the box in perf or other tooling, so this would essentially reset the clock (albeit only for code using these types...). |
|
If the expectation is that there'll be more additions like this (e.g. the |
|
I'd like to echo the above points as a soft concern (I'm not convinced to check my box, but wouldn't block if it would otherwise land). I think if we foresee future changes like this, we should consider that doing this dance every time (FCPing, waiting for downstream support, thinking about backwards-compatibility, etc.) is not ideal and that there may be better approaches (like emitting regular paths mentioned above). |
|
Given the above conversation I guess I'm going to file a concern on my own FCP 🙂 @rfcbot concern Compatibility with existing tools The extremely long latency before downstream tooling like If noboby objects, I'm going to cancel the FCP and suggest that we just use |
|
Here's a proposal for solving these kinds of problems in a more structured way: rust-lang/compiler-team#737 |
|
@michaelwoerister proposal cancelled. |
rfcbot
removed
proposed-final-comment-period
As discussed at <rust-lang#122106>, use the crate encoding to represent new primitives.
|
Thanks everyone for the feedback, I have opened #123816 to supercede this. |
As discussed at <rust-lang#122106>, use the crate encoding to represent new primitives.
As discussed at <rust-lang#122106>, use the crate encoding to represent new primitives.
As discussed at <rust-lang#122106>, use the crate encoding to represent new primitives.
…lwoerister Add v0 symbol mangling for `f16` and `f128` As discussed at <rust-lang#122106>, use the crate encoding to represent new primitives.
As discussed at <rust-lang#122106>, use the crate encoding to represent new primitives. (cherry picked from commit 809b84e)
…r-demangling-recommendation, r=davidtwco Recommend to never display zero disambiguators when demangling v0 symbols This PR extends the [v0 symbol mangling documentation](https://doc.rust-lang.org/rustc/symbol-mangling/v0.html) with the strong recommendation that demanglers should never display zero-disambiguators, especially when dealing with `crate-root`. Being able to rely on `C3foo` to be rendered as `foo` (i.e. without explicit disambiguator value) rather than as `foo[0]` allows the compiler to encode things like new basic types in a backward compatible way. This idea has been originally proposed by `@eddyb` in [the discussion around supporting `f16` and `f128` in the v0 mangling scheme](rust-lang#122106). It is a generally useful mechanism for supporting a certain class of new elements in the v0 mangling scheme in a backward compatible way (whether as a temporary workaround until downstream tooling has picked up grammar changes or as a permanent encoding). cc `@tgross35`
Rollup merge of rust-lang#124514 - michaelwoerister:zero-disambiguator-demangling-recommendation, r=davidtwco Recommend to never display zero disambiguators when demangling v0 symbols This PR extends the [v0 symbol mangling documentation](https://doc.rust-lang.org/rustc/symbol-mangling/v0.html) with the strong recommendation that demanglers should never display zero-disambiguators, especially when dealing with `crate-root`. Being able to rely on `C3foo` to be rendered as `foo` (i.e. without explicit disambiguator value) rather than as `foo[0]` allows the compiler to encode things like new basic types in a backward compatible way. This idea has been originally proposed by `@eddyb` in [the discussion around supporting `f16` and `f128` in the v0 mangling scheme](rust-lang#122106). It is a generally useful mechanism for supporting a certain class of new elements in the v0 mangling scheme in a backward compatible way (whether as a temporary workaround until downstream tooling has picked up grammar changes or as a permanent encoding). cc `@tgross35`
As discussed at <rust-lang/rust#122106>, use the crate encoding to represent new primitives. (cherry picked from commit 809b84e)
This establishes new character encodings for the new primitive types tracked at #116909:
kforf16qforf128We cannot easily be consistent with Itanium because it uses a multi-character encoding for
_Float16(Dh), which I believe would conflict withdynanyway. We could use Itanium'sgforf128, butqis more consistent withf=float=f32,d=double=f64,q=quad=f128(unfortunatelyhforhalf/f16is already taken).Per @michaelwoerister this will need a compiler FCP
See also previous discussion at rust-lang/rustc-demangle#64. Currently, the compiler will just ICE if attempting to v0 mangle these types.
@rustbot label +T-compiler +needs-fcp
Cc @rcvalle @ehuss