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.

Sign up for GitHub

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

Jump to bottom

Refactor the abi handling code a bit #79067

Merged
merged 5 commits into from
Nov 21, 2020
Merged

Refactor the abi handling code a bit #79067

merged 5 commits into from
Nov 21, 2020

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Nov 15, 2020

I am not quite sure if all changes are improvements.

@bjorn3 bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 15, 2020
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2020
@bjorn3 bjorn3 force-pushed the abi_refactor branch 2 times, most recently from c75dd3f to c4aa9d9 Compare November 15, 2020 10:13
@bjorn3 bjorn3 marked this pull request as draft November 15, 2020 10:24
@bjorn3 bjorn3 force-pushed the abi_refactor branch 3 times, most recently from 9a81fa8 to 0c7827f Compare November 15, 2020 10:54
@bjorn3 bjorn3 marked this pull request as ready for review November 15, 2020 11:45
@bjorn3
Copy link
Member Author

bjorn3 commented Nov 15, 2020 •
edited
Loading

I mixed up the apply_llfn and apply_callsite methods a few times. It is now fixed. I also marked them as unsafe, as the mixup caused the process to abort.

lcnr
lcnr reviewed Nov 15, 2020
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

Most of this looks good to me but don't know enough about this.

Really like the commit structure ❤️

r? @nagisa maybe

compiler/rustc_target/src/abi/call/mod.rs Outdated Show resolved Hide resolved
@lcnr lcnr assigned nagisa and unassigned lcnr Nov 15, 2020
@lcnr
Copy link
Contributor

lcnr commented Nov 15, 2020

forgot that bors ignores review comments

@bors
Copy link
Contributor

bors commented Nov 19, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

nagisa
nagisa approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@nagisa nagisa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I only have concerns with the last commit. The arguments they take are &Value (i.e. must be valid pointers to a Value by construction) and the crash mentioned in the commit was more likely a downcast assertion, rather than a genuine memory safety problem.

The rest LGTM

compiler/rustc_target/src/abi/call/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/abi.rs Outdated Show resolved Hide resolved
@bjorn3
Replace ZExt and SExt flags with ArgExtension enum
967a228 
Both flags are mutually exclusive
@bjorn3
Copy link
Member Author

bjorn3 commented Nov 21, 2020

I only have concerns with the last commit. The arguments they take are &Value (i.e. must be valid pointers to a Value by construction) and the crash mentioned in the commit was more likely a downcast assertion, rather than a genuine memory safety problem.

Dropped that commit

@bjorn3 bjorn3 force-pushed the abi_refactor branch 2 times, most recently from adc1c7e to a833e6a Compare November 21, 2020 18:16
@nagisa
Copy link
Member

nagisa commented Nov 21, 2020

r=me with db6aa9f squashed away.

@bjorn3
Replace ByVal attribute with on_stack field for Indirect
42b0b80 
This makes it clearer that only PassMode::Indirect allows ByVal
@bjorn3
Remove StructRet arg attr
39b8b2b 
It is applied exactly when the return value has an indirect pass mode.
Except for InReg on x86 fastcall, arg attrs are now only used for
optimization purposes and thus are fine to ignore.
@bjorn3
Rename prefix_chunk to prefix_chunk_size
6a5f537
@bjorn3
Copy link
Member Author

bjorn3 commented Nov 21, 2020

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Nov 21, 2020

📌 Commit 43968aa has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2020
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Nov 21, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2020
@bors
Auto merge of rust-lang#79273 - Dylan-DPC:rollup-zd10xlt, r=Dylan-DPC
da38469 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77844 (clarify rules for ZST Boxes)
 - rust-lang#79067 (Refactor the abi handling code a bit)
 - rust-lang#79182 (Fix links to extern types in rustdoc (fixes rust-lang#78777))
 - rust-lang#79231 (Exhaustively match in variant count instrinsic)
 - rust-lang#79238 (Direct RUSTC_LOG (tracing/log) output to stderr instead of stdout.)
 - rust-lang#79256 (Fix typos)
 - rust-lang#79264 (Get rid of some doctree items)
 - rust-lang#79272 (Support building clone shims for arrays with generic size)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4268357 into rust-lang:master Nov 21, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 21, 2020
@bjorn3 bjorn3 deleted the abi_refactor branch November 21, 2020 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

@nagisa nagisa nagisa approved these changes

Assignees

@nagisa nagisa

Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bjorn3 @rust-highfive @lcnr @bors @nagisa @rustbot