Skip to content

compiler: doc/comment some codegen-for-functions interfaces #143716

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
Merged
merged 2 commits into from
Jul 12, 2025
Merged

compiler: doc/comment some codegen-for-functions interfaces #143716

merged 2 commits into from
Jul 12, 2025

Conversation

workingjubilee
Copy link
Member

An out-of-date comment gets updated and some underdocumented functions get documented.

@workingjubilee
compiler: recomment needs_fn_once_adapter_shim
04bb68a 
This requires digging up ffee956
and reading the comments there to understand that the callee in
resolve_closure previously directly handled a function pointer value.
@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2025

r? @fee1-dead

rustbot has assigned @fee1-dead.
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 Jul 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@workingjubilee
Copy link
Member Author

r? @WaffleLapkin or reroll idk it's just some comments

@rustbot rustbot assigned WaffleLapkin and unassigned fee1-dead Jul 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2025

WaffleLapkin is currently at their maximum review capacity.
They may take a while to respond.

@workingjubilee workingjubilee force-pushed the document-some-codegen-backend-stuff branch from 143d057 to afdd2a9 Compare July 10, 2025 04:27
@fee1-dead
Copy link
Member

r? fee1-dead
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 10, 2025

📌 Commit afdd2a9 has been approved by fee1-dead

It is now in the queue for this repository.

@rustbot rustbot assigned fee1-dead and unassigned WaffleLapkin Jul 10, 2025
@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 Jul 10, 2025
bjorn3
bjorn3 reviewed Jul 10, 2025
View reviewed changes
compiler/rustc_codegen_ssa/src/traits/builder.rs Outdated
/// "Finally codegen the call"
///
/// The typical case that an argument is None is during the codegen of intrinsics,
/// as they are "fake functions" that have no meaningful ABI.
Copy link
Member

Choose a reason for hiding this comment

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

Also callng lang items uses a None FnAbi. The FnAbi and Instance passed here are only used for CFI. The caller is expected to have already handled the ABI lowering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, so there's no particular reason that our call code looks like this except for CFI?

Copy link
Member

Choose a reason for hiding this comment

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

It is also used for applying some call-site attributes like nounwind or noreturn, but yeah, most of the code in it is for CFI

Copy link
Member Author

Choose a reason for hiding this comment

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

@bjorn3 Can you please take another look at the latest version to see if I accurately described the situation?

Copy link
Member Author

Choose a reason for hiding this comment

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

re: the other call-site attributes, my understanding is that applying these attributes would not be required for the sake of correctness.

@workingjubilee
Copy link
Member Author

Updating to include info from the comments @bjorn3 gave.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 10, 2025
@workingjubilee workingjubilee force-pushed the document-some-codegen-backend-stuff branch 2 times, most recently from 72aec74 to 7f0be7e Compare July 10, 2025 21:20
@workingjubilee workingjubilee mentioned this pull request Jul 10, 2025
Open
@workingjubilee workingjubilee added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 11, 2025
bjorn3
bjorn3 reviewed Jul 11, 2025
View reviewed changes
compiler/rustc_codegen_ssa/src/traits/builder.rs Outdated
/// in the context of other compiler-enhanced security features.
///
/// The typical case that they are None is during the codegen of intrinsics and lang-items,
/// as those are "fake functions" that have no meaningful ABI, et cetera.
Copy link
Member

Choose a reason for hiding this comment

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

Lang items do have a meaningful ABI. It just happens to match the most naive implementation you can think of as they normally only have primitive arguments and no return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

"trivial", then.

@workingjubilee
compiler: comment on some call-related codegen fn in cg_ssa
39f7707 
Partially documents the situation due to LLVM CFI.
@workingjubilee workingjubilee force-pushed the document-some-codegen-backend-stuff branch from 7f0be7e to 39f7707 Compare July 11, 2025 08:08
@fee1-dead
Copy link
Member

r? bjorn3 to take a look at the language though it LGTM.

@rustbot rustbot assigned bjorn3 and unassigned fee1-dead Jul 11, 2025
@bjorn3
Copy link
Member

bjorn3 commented Jul 11, 2025

@bors r=bjorn3,fee1-dead

@bors
Copy link
Collaborator

bors commented Jul 11, 2025

📌 Commit 39f7707 has been approved by bjorn3,fee1-dead

It is now in the queue for this repository.

@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 Jul 11, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 11, 2025
@matthiaskrgr
Rollup merge of rust-lang#143716 - workingjubilee:document-some-codeg…
25b1232 
…en-backend-stuff, r=bjorn3,fee1-dead

compiler: doc/comment some codegen-for-functions interfaces

An out-of-date comment gets updated and some underdocumented functions get documented.
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 11, 2025
Closed
bors added a commit that referenced this pull request Jul 11, 2025
@bors
Auto merge of #143790 - matthiaskrgr:rollup-hslb9jq, r=matthiaskrgr
5e942ad 
Rollup of 10 pull requests

Successful merges:

 - #142301 (tests: Fix duplicated-path-in-error fail with musl)
 - #143403 (Port several trait/coherence-related attributes the new attribute system)
 - #143633 (fix: correct assertion to check for 'noinline' attribute presence before removal)
 - #143647 (Clarify and expand documentation for std::sys_common dependency structure)
 - #143716 (compiler: doc/comment some codegen-for-functions interfaces)
 - #143747 (Add target maintainer information for aarch64-unknown-linux-musl)
 - #143759 (Fix typos in function names in the `target_feature` test)
 - #143767 (Bump `src/tools/x` to Edition 2024 and some cleanups)
 - #143769 (Remove support for SwitchInt edge effects in backward dataflow)
 - #143770 (build-helper: clippy fixes)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 11, 2025
Merged
bors added a commit that referenced this pull request Jul 12, 2025
@bors
Auto merge of #143810 - matthiaskrgr:rollup-iw7a23z, r=matthiaskrgr
915e535 
Rollup of 9 pull requests

Successful merges:

 - #143403 (Port several trait/coherence-related attributes the new attribute system)
 - #143633 (fix: correct assertion to check for 'noinline' attribute presence before removal)
 - #143647 (Clarify and expand documentation for std::sys_common dependency structure)
 - #143716 (compiler: doc/comment some codegen-for-functions interfaces)
 - #143747 (Add target maintainer information for aarch64-unknown-linux-musl)
 - #143759 (Fix typos in function names in the `target_feature` test)
 - #143767 (Bump `src/tools/x` to Edition 2024 and some cleanups)
 - #143769 (Remove support for SwitchInt edge effects in backward dataflow)
 - #143770 (build-helper: clippy fixes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b18064f into rust-lang:master Jul 12, 2025
11 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 12, 2025
rust-timer added a commit that referenced this pull request Jul 12, 2025
@rust-timer
Unrolled build for #143716
9b3c2b2 
Rollup merge of #143716 - workingjubilee:document-some-codegen-backend-stuff, r=bjorn3,fee1-dead

compiler: doc/comment some codegen-for-functions interfaces

An out-of-date comment gets updated and some underdocumented functions get documented.
@workingjubilee workingjubilee deleted the document-some-codegen-backend-stuff branch July 12, 2025 15:59
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Jul 15, 2025
@bors
Auto merge of #143810 - matthiaskrgr:rollup-iw7a23z, r=matthiaskrgr
d51d10b 
Rollup of 9 pull requests

Successful merges:

 - rust-lang/rust#143403 (Port several trait/coherence-related attributes the new attribute system)
 - rust-lang/rust#143633 (fix: correct assertion to check for 'noinline' attribute presence before removal)
 - rust-lang/rust#143647 (Clarify and expand documentation for std::sys_common dependency structure)
 - rust-lang/rust#143716 (compiler: doc/comment some codegen-for-functions interfaces)
 - rust-lang/rust#143747 (Add target maintainer information for aarch64-unknown-linux-musl)
 - rust-lang/rust#143759 (Fix typos in function names in the `target_feature` test)
 - rust-lang/rust#143767 (Bump `src/tools/x` to Edition 2024 and some cleanups)
 - rust-lang/rust#143769 (Remove support for SwitchInt edge effects in backward dataflow)
 - rust-lang/rust#143770 (build-helper: clippy fixes)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@bjorn3 bjorn3

Labels
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.90.0
Development

Successfully merging this pull request may close these issues.
6 participants
@workingjubilee @rustbot @fee1-dead @bors @bjorn3 @WaffleLapkin