Skip to content

Allow section override when using patchable-function-entries#157445

Open
pmur wants to merge 1 commit into
rust-lang:mainfrom
pmur:murp/extend-patchable
Open

Allow section override when using patchable-function-entries#157445
pmur wants to merge 1 commit into
rust-lang:mainfrom
pmur:murp/extend-patchable

Conversation

@pmur

@pmur pmur commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Sometimes it is necessary to group patchable function entrypoint records in distinct linker sections. This is the case for some bpf functions within the linux kernel which shouldn't be visible to ftrace.

Extend -Zpatchable-function-entry to accept an argument of the form prefix_nops,total_nops,record_section, which places all entry record into a user specified section.

Likewise, extend the patchable_function_entry attribute to accept an optional section="name" option to place a function into a specific section.

This is made possible by llvm attribute patchable-function-entry-section added in llvm 21.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 4, 2026
@pmur pmur marked this pull request as ready for review June 4, 2026 21:52
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2026
@rustbot

rustbot commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 4, 2026
@rustbot

rustbot commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

r? @jackh726

rustbot has assigned @jackh726.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 19 candidates

@pmur

pmur commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

I think this is an important feature which we weren't aware of during the initial implementation of #123115.

Adding this should allow Rust to reduce the need to implement mcount or fentry (essentially x86 only) for compatibility within the linux kernel, and likely reduces the need for rust-lang/rfcs#3917.

@pmur

pmur commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot reroll

@rustbot rustbot assigned chenyukang and unassigned jackh726 Jun 17, 2026
@chenyukang

Copy link
Copy Markdown
Member

@rustbot reroll

@rustbot rustbot assigned mejrs and unassigned chenyukang Jun 22, 2026

@mejrs mejrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What are the rules for valid section names? This implementation accepts every string. Is #[patchable_function_entry(section = "")] valid? Can I use names with null bytes in them?

View changes since this review

/// Nops after the entry
entry: u8,
/// An optional section name to record the entry location
section: Option<String>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
section: Option<String>,
section: Option<Symbol>,

This will be simpler if you pass around the symbol (not String/&str) everywhere, just use as_str before calling llvm api.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is initialized before string interning is available. Or, so the runtime crash leads me to believe. Is there somewhere to insert a late initialization?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I wasn't aware of that, makes sense. I'd leave it as is then.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2026
@rustbot

rustbot commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

sym::section => {
// Duplicate entries are not allowed
if section.is_some() {
errored = true;

@mejrs mejrs Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: usually we use Option<ErrorGuaranteed> for "has errored" variables. Feel free to implement it or not, at your option.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can that be done within the attribute parsing framework? I think I am constrained to the SingleAttributeParser trait.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Functions like duplicate_key return ErrorGuaranteed for this reason.

@pmur

pmur commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

What are the rules for valid section names?

I think that gets punted to the assembler, and the targets object file format. For ELF, I think that is any nul terminated string.

@rust-bors

This comment has been minimized.

@mejrs

mejrs commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

What are the rules for valid section names?

I think that gets punted to the assembler, and the targets object file format. For ELF, I think that is any nul terminated string.

So this would fail at the codegen stage? It makes for a better user experience to reject it earlier, similar to #155817

@pmur pmur force-pushed the murp/extend-patchable branch from 8f7613b to c729ed3 Compare June 25, 2026 15:57
@rustbot

rustbot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@pmur

pmur commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Updated to handle nul's more gracefully. llvm doesn't handle the inline nul gracefully.

Sometimes it is necessary to group patchable function entrypoint
records in distinct linker sections. This is the case for some bpf
functions within the linux kernel which shouldn't be visible to
ftrace.

Extend `-Zpatchable-function-entry` to accept an argument of the form
`prefix_nops,total_nops,record_section`, which places all entry
record into a user specified section.

Likewise, extend the `patchable_function_entry` attribute to accept an
optional `section="name"` option to place a function into a specific
section.

This is made possible by llvm attribute `patchable-function-entry-section`
added in llvm 21.
@pmur pmur force-pushed the murp/extend-patchable branch from c729ed3 to 55b62aa Compare June 25, 2026 21:16
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  SKIP_SUBMODULES: src/gcc
  IMAGE: tidy
##[endgroup]
    Updating crates.io index
error: failed to get `askama_derive` as a dependency of package `askama_macros v0.16.0`
    ... which satisfies dependency `askama_macros = "=0.16.0"` of package `askama v0.16.0`
    ... which satisfies dependency `askama = "^0.16.0"` of package `citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)`

Caused by:
  failed to load source for dependency `askama_derive`

Caused by:
  unable to update registry `crates-io`

Caused by:
  download of as/ka/askama_derive failed

Caused by:
  curl failed

Caused by:

@mejrs

mejrs commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Updated to handle nul's more gracefully. llvm doesn't handle the inline nul gracefully.

Thanks, this looks good. What about empty strings? IIRC with link_name, llvm would just make up something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants