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

rust_for_linux: -Zreg-struct-return commandline flag for X86 (#116973) #130777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Sep 24, 2024

Command line flag -Zreg-struct-return for X86 (32-bit) for rust-for-linux.
This flag enables the same behavior as the abi_return_struct_as_int target spec key.

@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 24, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Darksonn
Copy link
Contributor

cc @ojeda

@azhogin azhogin marked this pull request as ready for review September 26, 2024 17:35
@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

These commits modify compiler targets.
(See the Target Tier Policy.)

@Darksonn
Copy link
Contributor

@rustbot label A-rust-for-linux
@rustbot label A-ABI

@rustbot rustbot added A-rust-for-linux Relevant for the Rust-for-Linux project A-ABI Area: Concerning the application binary interface (ABI) labels Sep 28, 2024
Comment on lines 34 to 38
// reg-struct-return doesn't work for "Rust" calling conv
// CHECK: { i32, i32 } @f2()
#[no_mangle]
pub extern "Rust" fn f2() -> Foo {
Foo { x: 1, y: 2 }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't it work for the Rust calling convention? Is that what we want? I assume that there's a reason we use this flag, but I don't know whether it's just a question of performance or if it's an important mitigation.

@andyhhp Do you know if this matters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At technical level, "external" abi's are processed a little in different way (another place in code). It is not a big problem to support the same behavior for "Rust" calling conv, I just need to know if it is required.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't it work for the Rust calling convention? Is that what we want? I assume that there's a reason we use this flag, but I don't know whether it's just a question of performance or if it's an important mitigation.

@andyhhp Do you know if this matters?

Rust and C need to agree on how to return >GPR sized values, if this happens to be used in either direction.

Whether that is via some bindings using the Rust calling conventions is something I can't answer.

The use of >GPR sized return values is rare in C because it's irritating to set up. You need a struct/typedef with a fairly precise layout, but the code generation improvements are substantial, hence why they do get used occasionally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Rust ABI is not stable. C should not call an extern "Rust" fn. This matters for extern "C" fn and a few others. And reusing this code, tuned for extern "C" and other foreign ABIs, for Rust ABI code? Proved incapable of compiling libcore in #130432.

We're not going to be scope-creeping this PR into extern "Rust". That can be addressed alongside the extensive cleanups of our ABI code that it will require.

@rust-log-analyzer

This comment has been minimized.

@ojeda
Copy link
Contributor

ojeda commented Oct 2, 2024

Cc @vincenzopalazzo who was interested in this.

@bors
Copy link
Contributor

bors commented Oct 10, 2024

☔ The latest upstream changes (presumably #131458) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot
Copy link
Contributor

I'm not in capacity to review this.
r? compiler

@rustbot rustbot assigned petrochenkov and unassigned cjgillot Oct 11, 2024
@petrochenkov
Copy link
Contributor

r? @workingjubilee

@workingjubilee
Copy link
Member

I have mutated #116973 into a tracking issue for this, so we do not need to open another one.

@workingjubilee workingjubilee added the A-CLI Area: Command-line interface (CLI) to the compiler label Oct 22, 2024
@bors
Copy link
Contributor

bors commented Oct 22, 2024

☔ The latest upstream changes (presumably #132020) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Oct 24, 2024

☔ The latest upstream changes (presumably #132079) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines 815 to 830
let is_indirect_not_on_stack = !matches!(arg.layout.abi, Abi::Aggregate { .. })
|| matches!(arg.mode, PassMode::Indirect { on_stack: false, .. });
assert!(is_indirect_not_on_stack, "is_indirect_not_on_stack: {:?}", arg);

let size = arg.layout.size;
if !arg.layout.is_unsized() && size <= Pointer(AddressSpace::DATA).size(cx) {
let ptr_size = Pointer(AddressSpace::DATA).size(cx);

// In x86 we may return 2x pointer sized struct as i64
let reg_struct_return_case = tcx.sess.target.arch == "x86"
&& arg_idx.is_none()
&& tcx.sess.opts.unstable_opts.reg_struct_return
&& abi != SpecAbi::RustIntrinsic;

if !arg.layout.is_unsized()
&& (size <= ptr_size || (reg_struct_return_case && (size <= 2 * ptr_size)))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of this code has moved into rustc_target.

@azhogin
Copy link
Contributor Author

azhogin commented Oct 29, 2024

Reverted changes for "Rust" calling conv (flag works only for extern "C"-like calling conventions). Added unstable-book record, error about wrong arch and related test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-CLI Area: Command-line interface (CLI) to the compiler A-rust-for-linux Relevant for the Rust-for-Linux project O-x86_32 Target: x86 processors, 32 bit (like i686-*) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants