Skip to content

Allow specifying alignment for functions#81234

Merged
bors merged 1 commit into
rust-lang:masterfrom
repnop:fn-alignment
Apr 6, 2021
Merged

Allow specifying alignment for functions#81234
bors merged 1 commit into
rust-lang:masterfrom
repnop:fn-alignment

Conversation

@repnop

@repnop repnop commented Jan 21, 2021

Copy link
Copy Markdown
Contributor

Fixes #75072

This allows the user to specify alignment for functions, which can be useful for low level work where functions need to necessarily be aligned to a specific value.

I believe the error cases not covered in the match are caught earlier based on my testing so I had them just return None.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive

Copy link
Copy Markdown
Contributor

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2021
@repnop

repnop commented Jan 21, 2021

Copy link
Copy Markdown
Contributor Author

Not sure how that submodule got added, but removed 🤔

@rust-log-analyzer

This comment has been minimized.

@lcnr

lcnr commented Jan 21, 2021

Copy link
Copy Markdown
Contributor

nominating for t-lang for now, I expect us to land this unstably first if we want to do so, but it does seem desirable to do so imo.

@lcnr lcnr added I-nominated T-lang Relevant to the language team labels Jan 21, 2021
@joshtriplett

joshtriplett commented Feb 9, 2021

Copy link
Copy Markdown
Member

This seems reasonable to me, to add on an unstable basis. (There needs to be a feature-gate added.)

I also think many codebases will want to set this for all functions using a compiler option (e.g. set in a Cargo profile). But supporting it on individual functions seems reasonable as well.

EDIT: We discussed this in today's lang-team meeting, and this was the general consensus:

  • Please add a feature-gate (e.g. func_align)
  • When this is merged, we'll need a tracking issue

@repnop

repnop commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

Awesome! I should hopefully have some time soon to do that :)

@joshtriplett

Copy link
Copy Markdown
Member

We discussed this in today's @rust-lang/lang meeting, and we're in favor of adding this to nightly, for experimentation, as soon as there's a feature-gate added.

@rfcbot fcp merge
@rfcbot concern feature-gate

@rfcbot

rfcbot commented Feb 16, 2021

Copy link
Copy Markdown

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added 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 Feb 16, 2021
@repnop repnop force-pushed the fn-alignment branch 2 times, most recently from 383465c to dd575a9 Compare February 17, 2021 19:41
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@repnop

repnop commented Feb 17, 2021

Copy link
Copy Markdown
Contributor Author

I think I added all the requests, any other changes that need made?

@joshtriplett

Copy link
Copy Markdown
Member

@rfcbot resolve feature-gate

Thank you!

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 23, 2021
@rfcbot

rfcbot commented Feb 23, 2021

Copy link
Copy Markdown

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors

bors commented Feb 23, 2021

Copy link
Copy Markdown
Collaborator

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

@Havvy

Havvy commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

Shouldn't this be going through the RFC process?

@bors

bors commented Apr 5, 2021

Copy link
Copy Markdown
Collaborator

✌️ @repnop can now approve this pull request

@repnop

repnop commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

@bors r=lcnr

@bors

bors commented Apr 5, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 448d076 has been approved by lcnr

@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 Apr 5, 2021
@bors

bors commented Apr 6, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 448d076 with merge a6e7a5a...

@bors

bors commented Apr 6, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing a6e7a5a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2021
@bors bors merged commit a6e7a5a into rust-lang:master Apr 6, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 6, 2021
@bors bors mentioned this pull request Apr 6, 2021
@RalfJung

RalfJung commented Apr 15, 2021

Copy link
Copy Markdown
Member

What are the language-level effects of this? Is this mainly a hint for how to put the compiled binary code together, or is his somehow observable inside Rust (e.g. by adding new kinds of UB)?

@repnop

repnop commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Is this mainly a hint for how to put the compiled binary code together

yes, its not intended to introduce any visible effects from within Rust, but to allow telling LLVM that the function needs to be aligned to a specific boundary for situations in which that's necessary and you can't rely on the native alignment placing it at an appropriate address (e.g. so it can be inserted into a hardware register, in my case)

by adding new kinds of UB

I think the only thing that may have been glossed over in this PR as it stands right now is being able to under-align functions, which I'll need to investigate to see if we need to add some additional checks to the #[repr(align(...))] as to not allow that to happen, or if LLVM will ignore the alignment hint and force it up to the native alignment since it would fulfill the request anyways (which seems like it may be the case? at least with some limited test in the playground)

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2021
…ulacrum

[beta] Bootstrap from stable

This is the follow up to master/beta promotion, as well as the first round of backports:

* Revert "Allow specifying alignment for functions rust-lang#81234"
* Revert rust-lang#85176 addition of clone_from for ManuallyDrop rust-lang#85758
* rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType rust-lang#84867
*  [beta] Update cargo rust-lang#86563

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow specifying function alignment