Check ABI target compatibility for function pointers #128784
Conversation
|
rustbot has assigned @compiler-errors. Use |
rustbot
added
S-waiting-on-review
|
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
|
Thinking about it and seeing how many tests already broke by this and this only showing up on specific targets, maybe this should only raise a future incompatibility warning? |
|
@compiler-errors could you trigger a try run for this PR? Just to check if there are more tests that fail when run on a different target |
Do you have any particular targets in mind? I added some common try-jobs to your PR description. We can run a first battery of try-jobs on the common targets. |
|
@bors try |
Check ABI target compatibility for function pointers Related tracking issue: rust-lang#87678 Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent. This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea. Also this might break existing code, because we now emit extra errors. Does this require a crater run? # Example ```rust // build with: --target=x86_64-unknown-linux-gnu // These raise E0570 extern "thiscall" fn foo() {} extern "thiscall" { fn bar() } // This did not raise any error fn baz(f: extern "thiscall" fn()) { f() } ``` try-job: aarch64-gnu try-job: aarch64-apple try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
Thanks you! Sorry I missed that I could have filtered out specific jobs. I'll try to fix this soon. |
Feel free to ping me if you need another run. |
|
Just saw the t-compiler/meeting discussion that mentioned this PR. And this comment by @compiler-errors
I think this PR actually fixes a Bug, we came across this while working on CMSE, but the problem seems to be bigger. ABIs are not checked at all for target compatibility when used with function pointers. Or did we miss something and not checking ABIs on function pointers is on purpose? Is there a valid use-case for using a function pointer with an ABI for a different target somewhere? |
|
Just so I don't forget to check them. The list of all usages of function pointers in rust-lang/rust: with ABIs listed in the reference✔️ This is gated by
✔️ Rustdoc seems fine with this even on ✔️ This is gated by ✔️ These were added by this PR ✔️ gated by ✔️ gated by ✔️ gated by ✔️ gated by ✔️ has with other ABIs✔️ only in comments ✔️ only in text ✔️ introduced by this PR ✔️ Checks for this error and ignores supported targets ✔️ Has ✔️ Has ✔️ Has ✔️ Has ✔️ Has ✔️ Has ✔️ Has ✔️ Has ✔️ Has If there is a better way then greping these I would be happy to learn about it 😉 |
tests/debuginfo/type-names.rs
Outdated
There was a problem hiding this comment.
I'm not sure if it is ok to change this test from fastcall (only allowed on x86) to system.
From what I understand this test only wants to check that the ABI actually show up in the debug output, and I would guess that system works just as nice for this.
|
I think I found all the places that use |
Check ABI target compatibility for function pointers Related tracking issue: rust-lang#87678 Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent. This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea. Also this might break existing code, because we now emit extra errors. Does this require a crater run? # Example ```rust // build with: --target=x86_64-unknown-linux-gnu // These raise E0570 extern "thiscall" fn foo() {} extern "thiscall" { fn bar() } // This did not raise any error fn baz(f: extern "thiscall" fn()) { f() } ``` # Places to check in the tests: * [x] Check if we can use another ABI here ``` tests/debuginfo/type-names.rs 20:// gdb-check:type = type_names::GenericStruct<type_names::Struct1, extern "fastcall" fn(isize) -> usize> 375: let generic_struct2: GenericStruct<Struct1, extern "fastcall" fn(isize) -> usize> = ``` try-job: aarch64-gnu try-job: aarch64-apple try-job: x86_64-msvc try-job: x86_64-mingw try-job: i686-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
|
Oh I missed that one, I think it is fixed now, tested locally with |
|
I think it'd be ok if we allowed you to declare-but-not-call functions that are not valid on your architecture (it strikes me as potentially quite annoying to not be able to do that, actually), but I think we should be consistent. If we were going to make that change I'd expect to make it everywhere. |
|
@rustbot labels -I-lang-nominated We discussed this in the meeting today... and it's now in FCP. |
rustbot
removed
the
I-lang-nominated
|
I agree that we might want to allow this for uncalled private functions (or downgrade to a lint), but we would probably want to make sure codegen never happens for those functions because it isn't clear what a backend would do with them. Looks like #87678 is the place for more discussion of this. @rfcbot reviewed |
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
|
@rustbot ready |
rustbot
added
the
S-waiting-on-review
RalfJung
removed
the
S-waiting-on-team
|
@compiler-errors wrote
But I'm going to let them do the r+ since this is quite the big PR and it did require some rebasing since the review. |
| Some(false) | None => { | ||
| tcx.node_span_lint(UNSUPPORTED_FN_PTR_CALLING_CONVENTIONS, hir_id, span, |lint| { | ||
| lint.primary_message( | ||
| "use of calling convention not supported on this target on function pointer", |
There was a problem hiding this comment.
This diagnostic is a bit awkward. Perhaps change it to:
"the calling convention {} is not supported on this target"
I think mentioning fn ptrs is also unnecessary, since it's also a warning for items (and soon an error).
|
@bors r+ |
bors
added
S-waiting-on-bors
|
@tdittr would you like to tweak this message as a follow-up, or should I? |
…iler-errors Check ABI target compatibility for function pointers Tracking issue: rust-lang#130260 Related tracking issue: rust-lang#87678 Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent. This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea. Also this might break existing code, because we now emit extra errors. Does this require a crater run? # Example ```rust // build with: --target=x86_64-unknown-linux-gnu // These raise E0570 extern "thiscall" fn foo() {} extern "thiscall" { fn bar() } // This did not raise any error fn baz(f: extern "thiscall" fn()) { f() } ``` # Open Questions * [x] Should this report a future incompatibility warning like rust-lang#87678 ? * [ ] Is this the best place to perform the check?
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#128784 (Check ABI target compatibility for function pointers) - rust-lang#130965 (make `Step` doc-comments more clear) - rust-lang#131239 (Don't assume traits used as type are trait objs in 2021 edition) - rust-lang#131277 (Handle `clippy` cases of `rustc::potential_query_instability` lint) - rust-lang#131503 (More clearly document Stdin::read_line) - rust-lang#131567 (Emit an error for unstable attributes that reference already stable features) - rust-lang#131599 (Shallowly match opaque key in storage) - rust-lang#131617 (remove const_cow_is_borrowed feature gate) Failed merges: - rust-lang#131616 (merge const_ipv4 / const_ipv6 feature gate into 'ip' feature gate) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128784 - tdittr:check-abi-on-fn-ptr, r=compiler-errors Check ABI target compatibility for function pointers Tracking issue: rust-lang#130260 Related tracking issue: rust-lang#87678 Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent. This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea. Also this might break existing code, because we now emit extra errors. Does this require a crater run? # Example ```rust // build with: --target=x86_64-unknown-linux-gnu // These raise E0570 extern "thiscall" fn foo() {} extern "thiscall" { fn bar() } // This did not raise any error fn baz(f: extern "thiscall" fn()) { f() } ``` # Open Questions * [x] Should this report a future incompatibility warning like rust-lang#87678 ? * [ ] Is this the best place to perform the check?
I will do it :) |
Tracking issue: #130260
Related tracking issue: #87678
Compatibility of an ABI for a target was previously only performed on function definitions and
externblocks. This PR adds it also to function pointers to be consistent.This might have broken some of the
tests/ui/depending on the platform, so a try run seems like a good idea.Also this might break existing code, because we now emit extra errors. Does this require a crater run?
Example
Open Questions
unsupported_calling_conventions#87678 ?