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

Tracking issue for the thiscall calling convention #42202

Closed
froydnj opened this issue May 24, 2017 · 49 comments · Fixed by #114562
Closed

Tracking issue for the thiscall calling convention #42202

froydnj opened this issue May 24, 2017 · 49 comments · Fixed by #114562
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@froydnj
Copy link
Contributor

froydnj commented May 24, 2017

Added in #42058, which fixes #42044 and related requests from other bugs.

This calling convention is the default calling convention for C++ instance methods in the (MSVC) Windows ABI. At the very least, bindgen (and its dependency syntex) need this to exist so they have a reasonable chance of representing C++ instance methods in an AST. thiscall is therefore important for the interop story between Rust and C++ on Windows.

I don't think you could actually write callable C++ instance methods in Rust for MSVC, since mangled names in MSVC contain characters that, AFAICT, are not legal in Rust identifiers ('?', for instance). But maybe there is cleverness around assigning assembly names to things that I am not aware of.

@ids1024
Copy link
Contributor

ids1024 commented May 25, 2017

But maybe there is cleverness around assigning assembly names to things that I am not aware of.

If this doesn't exist, perhaps it warrants an RFC? In particular some kind of attribute to specify the symbol name as an alternative to no_mangle.

@froydnj
Copy link
Contributor Author

froydnj commented May 25, 2017

If this doesn't exist, perhaps it warrants an RFC? In particular some kind of attribute to specify the symbol name as an alternative to no_mangle.

I think this sounds like an excellent idea!

@ids1024
Copy link
Contributor

ids1024 commented May 25, 2017

It looks like there's already an export_name attribute that does this. That probably would work.

@Mark-Simulacrum Mark-Simulacrum added B-unstable Feature: Implemented in the nightly compiler and unstable. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 23, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@steveklabnik
Copy link
Member

Triage: not aware of any movement on stabilizing this.

@whatisaphone
Copy link
Contributor

not aware of any movement on stabilizing this

I see what you did there, and I like it.

More seriously, I'm just swinging by to say I use this feature independently of bindgen, and I hope it's stabilized one day (or at least sticks around in some form)

@MSxDOS
Copy link

MSxDOS commented Sep 28, 2020

What's blocking stabilization of this?

@pravic
Copy link
Contributor

pravic commented Dec 24, 2020

The same question.

cc @dtolnay to route this to an appropriate person.

@super-continent
Copy link

Wondering the same myself, any updates on stabilizing this? Would be very nice to have access to in stable.

@zorftw
Copy link

zorftw commented Jul 26, 2021

It's been rather long, why is still unstable?

@chorman0773
Copy link
Contributor

Indeed. I have a use for this in my compiler project https://github.com/LightningCreations/lccc, which would allow me to write bindings to the C++ api in rust on MSVC (which I am able to do on other major compilers, but need thiscall for i386 windows).

@beepster4096
Copy link
Contributor

I think this feature should be stabilized.

Stabilization Report

Summary

The abi_thiscall feature allows code to export and link to functions with the thiscall abi. This is the abi used by default for C++ member functions on x86 MSVC, and is very important for C++ FFI.

Tests

This feature is tested in ui/extern/extern-thiscall.rs

Documentation PRs

@ChrisDenton
Copy link
Contributor

If/when this is fcp'd, would anyone be ready with the stabilization PR? @drmeepster maybe?

Not that I want to rush anyone. I understand people have other priorities and commitments, etc. But I am quite keen to see this stabilized as soon as realistically possible. 😀

@petrochenkov
Copy link
Contributor

The UNSUPPORTED_CALLING_CONVENTIONS lint needs to be turned into an error for thiscall before stabilization.
To do that support for Thiscall needs to be reduced to the self.arch == "x86" case in fn is_abi_supported.

@ChrisDenton
Copy link
Contributor

@petrochenkov Sorry, I'm a bit confused. Shouldn't it be treated the same as Stdcall and Fastcall? They're already stabilized,

@petrochenkov
Copy link
Contributor

@ChrisDenton
I think we should warn on the stable ones (#86231 (comment)), but for unstable conventions like thiscall we can proceed to immediately reporting an error instead.

@petrochenkov
Copy link
Contributor

So I thought a bit more about the bindgen use case.
Suppose it wants to generate cross-platform bindings for a library with class methods, then it will need to emit thiscall ABI on x86 and C ABI everywhere else and put these two cases under cfgs.

Perhaps it would be more convenient to have a generic ABI similary to system (stdcall on x86, C elsewhere) that would expand to thiscall on x86 and to C everywhere else.
It may be a new separate ABI, but it could be more reasonable to not multiply entities, and make thiscall itself that generic ABI.

Anyway, it's all compatible with the strict version implemented in #90241 that supports thiscall only on x86.

@nagisa
Copy link
Member

nagisa commented Jul 16, 2023

Yeah, this definitely sounds like a T-lang question than a T-compiler one. What would be in scope of the T-compiler here is an advice to T-lang on the implementation quality of this ABI (which is no better or worse than the other stable ABI options people can pick from today.)

(I’ll still leave it up to somebody from T-lang to cancel this FCP and start a new one for themselves)

@nagisa nagisa added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 16, 2023
@Trolldemorted
Copy link
Contributor

Should we @ them directly, or will they notice because it is now tagged T-lang?

@compiler-errors
Copy link
Member

I don't think T-lang can cancel an FCP that they're not participating. I can nominate it and cc @rust-lang/lang. Happy to cancel the FCP in favor if they feel like it's necessary.

@compiler-errors compiler-errors added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jul 16, 2023
@joshtriplett
Copy link
Member

@rfcbot poll T-lang Should we stabilize thiscall?

@rfcbot
Copy link

rfcbot commented Jul 18, 2023

Team member @joshtriplett has asked teams: T-lang, for consensus on:

Should we stabilize thiscall?

@nikomatsakis
Copy link
Contributor

Discussed in meeting. We are generally in favor of this. To avoid procedural pain we decided to just use rfcbot poll.

@joshtriplett joshtriplett removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jul 25, 2023
@compiler-errors
Copy link
Member

canceling fcp in favor of poll above

@rfcbot fcp cancel

@compiler-errors compiler-errors added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 7, 2023
@compiler-errors
Copy link
Member

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Aug 7, 2023

@compiler-errors proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 7, 2023
@compiler-errors compiler-errors added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 7, 2023
@bors bors closed this as completed in 06daa9e Aug 7, 2023
@joshtriplett
Copy link
Member

(The T-lang poll was meant to substitute for T-lang's inclusion in a multi-team FCP. I hope this wasn't taken as a substitute for making sure T-compiler was fine with this too.)

@compiler-errors
Copy link
Member

compiler-errors commented Aug 7, 2023

I interpreted:

  1. the reassignment of team tag to T-lang by @nagisa
  2. @oli-obk's r+ in stabilize abi_thiscall #114562

as saying "T-lang's decision, not ours".

At this point, if any @rust-lang/compiler team member thinks that we should have continued with that FCP, they should speak up, since we'll have to do a retroactive FCP at this point (and/or unland that PR). I personally think that an FCP is unnecessary from our side, though.

github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 8, 2023
stabilize abi_thiscall

Closes rust-lang/rust#42202, stabilizing the use of the "thiscall" ABI.

FCP was substituted by a poll, and the poll has been accepted.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Aug 21, 2023
stabilize abi_thiscall

Closes rust-lang/rust#42202, stabilizing the use of the "thiscall" ABI.

FCP was substituted by a poll, and the poll has been accepted.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Oct 17, 2023
stabilize abi_thiscall

Closes rust-lang/rust#42202, stabilizing the use of the "thiscall" ABI.

FCP was substituted by a poll, and the poll has been accepted.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
stabilize abi_thiscall

Closes rust-lang/rust#42202, stabilizing the use of the "thiscall" ABI.

FCP was substituted by a poll, and the poll has been accepted.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
stabilize abi_thiscall

Closes rust-lang/rust#42202, stabilizing the use of the "thiscall" ABI.

FCP was substituted by a poll, and the poll has been accepted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.