Skip to content

add llvm writable attribute conditionally#155207

Open
quiode wants to merge 1 commit intorust-lang:mainfrom
quiode:writable
Open

add llvm writable attribute conditionally#155207
quiode wants to merge 1 commit intorust-lang:mainfrom
quiode:writable

Conversation

@quiode
Copy link
Copy Markdown

@quiode quiode commented Apr 12, 2026 •
edited by rustbot
Loading

View all comments

This PR tries to address rust-lang/unsafe-code-guidelines#584 (comment). It is part of a bachelor thesis supervised by @JoJoDeveloping and @RalfJung, for more information, see: Project_Description.pdf.
If the new -Zllvm-writable flag is set, the llvm writable attribute is inserted for all mutable borrows. This can be conditionally turned off on a per-function basis using the #[rustc_no_writable] attribute. The new Undefined Behaviour introduced by this can detected by Miri, which is implemented here: rust-lang/miri#4947.

Two library functions already received the #[rustc_no_writable] attribute, as they are known to cause problems under the Tree Borrows aliasing model with implicit writes enabled.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 12, 2026

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

@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-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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 12, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 12, 2026

r? @mati865

rustbot has assigned @mati865.
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 69 candidates
  • Random selection from 12 candidates

@quiode
Copy link
Copy Markdown
Author

quiode commented Apr 12, 2026

r? @RalfJung

@rustbot rustbot assigned RalfJung and unassigned mati865 Apr 12, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 12, 2026

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@rustbot

This comment has been minimized.

Sign in to view
RalfJung
RalfJung reviewed Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@RalfJung RalfJung left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This generally looks good, thanks! I just hope this larger ArgAttribute won't cause us problems. Cc @nikic for the LLVM parts, in case you want to take a 2nd look.

I am not sure if there's anything special to look out for in the new attribute infrastructure; @jdonszelmann would be great if you could take a brief look at that part.

@rustbot author

View changes since this review

Comment thread compiler/rustc_feature/src/builtin_attrs.rs Outdated
Comment thread compiler/rustc_session/src/options.rs Outdated
Comment thread compiler/rustc_target/src/callconv/mod.rs
// The subset of llvm::Attribute needed for arguments, packed into a bitfield.
#[derive(Clone, Copy, Default, Hash, PartialEq, Eq, HashStable_Generic)]
pub struct ArgAttribute(u8);
pub struct ArgAttribute(u16);
Copy link
Copy Markdown
Member

@RalfJung RalfJung Apr 14, 2026

Choose a reason for hiding this comment

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

Ah, this is unfortunate. We should benchmark this to ensure the larger bitfield isn't a problem.

Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated
Comment thread compiler/rustc_ty_utils/src/abi.rs
Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated
Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated
Comment thread src/doc/unstable-book/src/compiler-flags/llvm-writable.md Outdated
Comment thread tests/codegen-llvm/llvm-writable.rs
@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 Apr 14, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 14, 2026

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

JonathanBrouwer
JonathanBrouwer reviewed Apr 14, 2026
View reviewed changes
Comment thread compiler/rustc_attr_parsing/src/attributes/rustc_internal.rs Outdated
JonathanBrouwer
JonathanBrouwer approved these changes Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@JonathanBrouwer JonathanBrouwer left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

r=me on the attribute changes, didn't check the other part of this PR

View changes since this review

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Apr 14, 2026

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 14, 2026
rust-bors bot pushed a commit that referenced this pull request Apr 14, 2026
@bors
Auto merge of #155207 - quiode:writable, r=<try>
37fa495 
add llvm writable attribute conditionally
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 14, 2026

💔 Test for 37fa495 failed: CI. Failed job:

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Apr 14, 2026

^ The PR needs to be rebased

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 15, 2026

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.

@quiode
Copy link
Copy Markdown
Author

quiode commented Apr 15, 2026

@rustbot ready

@rustbot rustbot 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 Apr 15, 2026
@quiode quiode mentioned this pull request Apr 15, 2026
Open
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Apr 16, 2026

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors bot pushed a commit that referenced this pull request Apr 16, 2026
@bors
Auto merge of #155207 - quiode:writable, r=<try>
84b05e7 
add llvm writable attribute conditionally
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Apr 16, 2026 •
edited
Loading

This looks great, thanks! Please squash the commits.

Let's hope the larger ArgAttribute does not cause a perf issue -- I kicked of a benchmark run to check this.

@quiode
Copy link
Copy Markdown
Author

quiode commented Apr 16, 2026

@bors squash msg="add llvm writable attribute conditionally"

@rust-bors

This comment has been minimized.

Sign in to view
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 16, 2026

🔨 11 commits were squashed into da2bbfb.

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Apr 16, 2026

Nice, I've not seen this in action before. :)

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 16, 2026

☀️ Try build successful (CI)
Build commit: 84b05e7 (84b05e789a7194a627e1ca3cda715fe05e5ca3f6, parent: e8e4541ff19649d95afab52fdde2c2eaa6829965)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Copy Markdown
Collaborator

rust-timer commented Apr 16, 2026

Finished benchmarking commit (84b05e7): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 3.1%, secondary 1.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 3.1% [2.6%, 3.7%] 2
Regressions ❌
(secondary)		 4.5% [2.8%, 6.3%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.0% [-4.0%, -4.0%] 1
All ❌✅ (primary) 3.1% [2.6%, 3.7%] 2

Cycles

Results (primary 2.3%, secondary 1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 2.3% [2.2%, 2.4%] 2
Regressions ❌
(secondary)		 2.7% [1.8%, 4.4%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.6% [-3.6%, -3.6%] 1
All ❌✅ (primary) 2.3% [2.2%, 2.4%] 2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.756s -> 493.416s (0.54%)
Artifact size: 394.16 MiB -> 394.14 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 16, 2026
@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Apr 16, 2026

That looks good! This is just a secondary benchmark that has been rather noisy recently already, nothing to worry about IMO.

@bors r+

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 16, 2026

📌 Commit da2bbfb has been approved by RalfJung

It is now in the queue for this repository.

@rust-bors rust-bors bot 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 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

@JonathanBrouwer JonathanBrouwer JonathanBrouwer approved these changes

Assignees

@RalfJung RalfJung

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@quiode @rustbot @JonathanBrouwer @rust-timer @rust-log-analyzer @RalfJung @mati865