-
Notifications
You must be signed in to change notification settings - Fork 14k
error when ABI does not support guaranteed tail calls #148878
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
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
c96259d to
77632f7
Compare
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.
r=me with nits
re: checking this in hir_typeck, I'm working on a PR to move all tail call checks earlier, so :)
77632f7 to
78beefe
Compare
| Self::C { .. } | ||
| | Self::System { .. } | ||
| | Self::Rust | ||
| | Self::RustCall | ||
| | Self::RustCold | ||
| | Self::RustInvalid | ||
| | Self::Unadjusted | ||
| | Self::EfiApi | ||
| | Self::Aapcs { .. } | ||
| | Self::Cdecl { .. } | ||
| | Self::Stdcall { .. } | ||
| | Self::Fastcall { .. } | ||
| | Self::Thiscall { .. } | ||
| | Self::Vectorcall { .. } | ||
| | Self::SysV64 { .. } | ||
| | Self::Win64 { .. } => true, |
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.
When stabilization comes closer, we might want to do something like what c-variadic does above and classify specifically for which ABIs tail calls are stabilized. Because I don't think we (at least currently) test all of these ABIs thoroughly.
|
nits fixed, so @bors r=WaffleLapkin |
…bi, r=WaffleLapkin error when ABI does not support guaranteed tail calls Some ABIs cannot support guaranteed tail calls. There isn't really an exhaustive list, so this is a best effort. Conveniently, we already disallow calling most of these directly anyway. The only exception that I was able to trigger an LLVM assertion with so far was `cmse-nonsecure-entry`. For that calling convention, LLVM specifically notes that (guaranteed) tail calls cannot be supported: https://github.com/llvm/llvm-project/blob/28dbbba6c3a4e026e085c48cc022cb97b5d8bc6d/llvm/lib/Target/ARM/ARMISelLowering.cpp#L2331-L2335 --- I have some doubts about the implementation here though. I think it would be nicer to use `CanonAbi`, and move the `become` ABI check into `rustc_hir_typeck`, similar to `check_call_abi`: https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/callee.rs#L157-L194 Both the check for whether an ABI is callable and whether it supports guaranteed tail calls can then be methods (containing exhaustive matches) on `CanonAbi`. I'm however not sure - if the ABI checks are deliberately only performed when constructing MIR - what assumptions can be made about the `call` expression in [`check_expr_become`](https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/expr.rs#L1126-L1150), it looks like currently the check that the "argument" to `become` is a function call also only occurs later during MIR construction Are there issues with validating the ABI earlier in `rustc_hir_typeck` that I'm overlooking? I believe that we should already know the call's ABI and whether it is c-variadic at that point. cc `@workingjubilee` for `CanonAbi`, `@davidtwco` for cmse r? `@WaffleLapkin`
…bi, r=WaffleLapkin error when ABI does not support guaranteed tail calls Some ABIs cannot support guaranteed tail calls. There isn't really an exhaustive list, so this is a best effort. Conveniently, we already disallow calling most of these directly anyway. The only exception that I was able to trigger an LLVM assertion with so far was `cmse-nonsecure-entry`. For that calling convention, LLVM specifically notes that (guaranteed) tail calls cannot be supported: https://github.com/llvm/llvm-project/blob/28dbbba6c3a4e026e085c48cc022cb97b5d8bc6d/llvm/lib/Target/ARM/ARMISelLowering.cpp#L2331-L2335 --- I have some doubts about the implementation here though. I think it would be nicer to use `CanonAbi`, and move the `become` ABI check into `rustc_hir_typeck`, similar to `check_call_abi`: https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/callee.rs#L157-L194 Both the check for whether an ABI is callable and whether it supports guaranteed tail calls can then be methods (containing exhaustive matches) on `CanonAbi`. I'm however not sure - if the ABI checks are deliberately only performed when constructing MIR - what assumptions can be made about the `call` expression in [`check_expr_become`](https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/expr.rs#L1126-L1150), it looks like currently the check that the "argument" to `become` is a function call also only occurs later during MIR construction Are there issues with validating the ABI earlier in `rustc_hir_typeck` that I'm overlooking? I believe that we should already know the call's ABI and whether it is c-variadic at that point. cc ``@workingjubilee`` for `CanonAbi`, ``@davidtwco`` for cmse r? ``@WaffleLapkin``
Rollup of 14 pull requests Successful merges: - #146978 (Emit error when using path-segment keyword as cfg pred) - #148543 (Correctly link to associated trait items in reexports) - #148808 (Some resolve cleanups) - #148812 (coverage: Associate hole spans with expansion tree nodes ) - #148826 (CStr docs: Fix CStr vs &CStr confusion) - #148850 (Implement `Read::read_array`) - #148867 (Refactor `Box::take`) - #148870 (Remove unused LLVMModuleRef argument) - #148878 (error when ABI does not support guaranteed tail calls) - #148901 (Disable rustdoc-test-builder test partially for SGX target.) - #148902 (add missing s390x target feature to std detect test) - #148904 (waffle: stop watching codegen ssa) - #148906 (Expose fmt::Arguments::from_str as unstable.) - #148907 (add assembly test for infinite recursion with `become`) r? `@ghost` `@rustbot` modify labels: rollup
…bi, r=WaffleLapkin error when ABI does not support guaranteed tail calls Some ABIs cannot support guaranteed tail calls. There isn't really an exhaustive list, so this is a best effort. Conveniently, we already disallow calling most of these directly anyway. The only exception that I was able to trigger an LLVM assertion with so far was `cmse-nonsecure-entry`. For that calling convention, LLVM specifically notes that (guaranteed) tail calls cannot be supported: https://github.com/llvm/llvm-project/blob/28dbbba6c3a4e026e085c48cc022cb97b5d8bc6d/llvm/lib/Target/ARM/ARMISelLowering.cpp#L2331-L2335 --- I have some doubts about the implementation here though. I think it would be nicer to use `CanonAbi`, and move the `become` ABI check into `rustc_hir_typeck`, similar to `check_call_abi`: https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/callee.rs#L157-L194 Both the check for whether an ABI is callable and whether it supports guaranteed tail calls can then be methods (containing exhaustive matches) on `CanonAbi`. I'm however not sure - if the ABI checks are deliberately only performed when constructing MIR - what assumptions can be made about the `call` expression in [`check_expr_become`](https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/expr.rs#L1126-L1150), it looks like currently the check that the "argument" to `become` is a function call also only occurs later during MIR construction Are there issues with validating the ABI earlier in `rustc_hir_typeck` that I'm overlooking? I believe that we should already know the call's ABI and whether it is c-variadic at that point. cc ```@workingjubilee``` for `CanonAbi`, ```@davidtwco``` for cmse r? ```@WaffleLapkin```
Rollup of 15 pull requests Successful merges: - #148543 (Correctly link to associated trait items in reexports) - #148808 (Some resolve cleanups) - #148812 (coverage: Associate hole spans with expansion tree nodes ) - #148826 (CStr docs: Fix CStr vs &CStr confusion) - #148850 (Implement `Read::read_array`) - #148867 (Refactor `Box::take`) - #148870 (Remove unused LLVMModuleRef argument) - #148878 (error when ABI does not support guaranteed tail calls) - #148901 (Disable rustdoc-test-builder test partially for SGX target.) - #148902 (add missing s390x target feature to std detect test) - #148904 (waffle: stop watching codegen ssa) - #148906 (Expose fmt::Arguments::from_str as unstable.) - #148907 (add assembly test for infinite recursion with `become`) - #148928 (Move & adjust some `!`-adjacent tests) - #148929 (ignore `build-rust-analyzer` even if it's a symlink) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #148878 - folkertdev:tail-call-unsupported-abi, r=WaffleLapkin error when ABI does not support guaranteed tail calls Some ABIs cannot support guaranteed tail calls. There isn't really an exhaustive list, so this is a best effort. Conveniently, we already disallow calling most of these directly anyway. The only exception that I was able to trigger an LLVM assertion with so far was `cmse-nonsecure-entry`. For that calling convention, LLVM specifically notes that (guaranteed) tail calls cannot be supported: https://github.com/llvm/llvm-project/blob/28dbbba6c3a4e026e085c48cc022cb97b5d8bc6d/llvm/lib/Target/ARM/ARMISelLowering.cpp#L2331-L2335 --- I have some doubts about the implementation here though. I think it would be nicer to use `CanonAbi`, and move the `become` ABI check into `rustc_hir_typeck`, similar to `check_call_abi`: https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/callee.rs#L157-L194 Both the check for whether an ABI is callable and whether it supports guaranteed tail calls can then be methods (containing exhaustive matches) on `CanonAbi`. I'm however not sure - if the ABI checks are deliberately only performed when constructing MIR - what assumptions can be made about the `call` expression in [`check_expr_become`](https://github.com/rust-lang/rust/blob/d6deffe2debecc66501e50f9573214139ab4d678/compiler/rustc_hir_typeck/src/expr.rs#L1126-L1150), it looks like currently the check that the "argument" to `become` is a function call also only occurs later during MIR construction Are there issues with validating the ABI earlier in `rustc_hir_typeck` that I'm overlooking? I believe that we should already know the call's ABI and whether it is c-variadic at that point. cc ````@workingjubilee```` for `CanonAbi`, ````@davidtwco```` for cmse r? ````@WaffleLapkin````
Some ABIs cannot support guaranteed tail calls. There isn't really an exhaustive list, so this is a best effort. Conveniently, we already disallow calling most of these directly anyway. The only exception that I was able to trigger an LLVM assertion with so far was
cmse-nonsecure-entry.For that calling convention, LLVM specifically notes that (guaranteed) tail calls cannot be supported:
https://github.com/llvm/llvm-project/blob/28dbbba6c3a4e026e085c48cc022cb97b5d8bc6d/llvm/lib/Target/ARM/ARMISelLowering.cpp#L2331-L2335
I have some doubts about the implementation here though. I think it would be nicer to use
CanonAbi, and move thebecomeABI check intorustc_hir_typeck, similar tocheck_call_abi:rust/compiler/rustc_hir_typeck/src/callee.rs
Lines 157 to 194 in d6deffe
Both the check for whether an ABI is callable and whether it supports guaranteed tail calls can then be methods (containing exhaustive matches) on
CanonAbi. I'm however not surecallexpression incheck_expr_become, it looks like currently the check that the "argument" tobecomeis a function call also only occurs later during MIR constructionAre there issues with validating the ABI earlier in
rustc_hir_typeckthat I'm overlooking? I believe that we should already know the call's ABI and whether it is c-variadic at that point.cc @workingjubilee for
CanonAbi, @davidtwco for cmser? @WaffleLapkin