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.

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

distinct 'static' items never overlap #1657

Merged
merged 4 commits into from
Jan 25, 2025
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 20, 2024

It seems like so far we did not actually guarantee this.

While we are at it, also clarify that static initializers can read even mutable statics, and what happens in that case.

@workingjubilee
Copy link
Member

I don't think you understood my example? I was positing that this seems to be a legal interpretation of the reference's current text:

static ZEE: u8 = 0;
static ZED: u8 = 0;
assert_eq!(&raw const ZEE, &raw const ZEE);
assert_eq!(&raw const ZED, &raw const ZED);
assert_eq!(&raw const ZEE, &raw const ZED);

@workingjubilee
Copy link
Member

Because this

I would say if a static has a wibbly wobbly address not equal to itself, that's not a "precise memory location". We don't even have addresses that are not equal to themselves.

cannot be a response to what I actually said if it is said with an understanding of what I was trying to say. :/

@RalfJung
Copy link
Member Author

RalfJung commented Oct 20, 2024

I think I understood the example? I don't understand how that can be a valid interpretation of the text. A static has a location, &raw const gives you a pointer pointing there. Different locations compare inequal.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 20, 2024

Oh, maybe I didn't quite understand the example.

But statics are certainly intended to be unique and disjoint. That's their point -- they describe a place, distinct from all other places.

More specifically, statics form their own allocated objects that don't overlap with any other allocated object. So in fact ZST statics are not quite unique -- but statics of type i32 are guaranteed to be at least 4 apart.

@RalfJung
Copy link
Member Author

@rustbot label +T-opsem

@rustbot rustbot added the T-opsem Team: opsem label Oct 20, 2024
@RalfJung
Copy link
Member Author

@rfcbot merge
since so far it seems like we haven't actually documented "different statics are disjoint".
Cc @rust-lang/lang

@rfcbot
Copy link

rfcbot commented Oct 20, 2024

Team member @RalfJung 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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@RalfJung RalfJung changed the title attempt to clarify 'static' unique address guarantees distinct 'static' items never overlap Oct 20, 2024
@JakobDegen
Copy link

Lgtm, with the note that it's important this language remains restricted to static items, and not other language constructs that produce statics - const promotion, vtables, functions, etc. Will check my box when I'm off mobile, as apparently the GH app doesn't let you edit things anymore (or someone can do it for me :) )

@RalfJung
Copy link
Member Author

You probably don't have edit rights here. I don't.

You can use @rfcbot reviewed to check your box.

with the note that it's important this language remains restricted to static items, and not other language constructs that produce statics - const promotion, vtables, functions, etc.

I would argue those aren't statics, they are other kinds of global allocations -- exactly because of this fundamental difference.

@JakobDegen
Copy link

@rfcbot reviewed

@workingjubilee
Copy link
Member

workingjubilee commented Oct 20, 2024

hmm. how must the addressing work for this, then?

static BLAH: &str = "blah";
static ALSO_BLAH: &str = "blah";

Are these potentially two different pointers to the same string literal?

@RalfJung
Copy link
Member Author

Are these potentially two different pointers to the same string literal?

Yes.

@saethlin
Copy link
Member

@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Oct 20, 2024

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

psst @RalfJung, I wasn't able to add the final-comment-period label, please do so.

@digama0
Copy link

digama0 commented Oct 20, 2024

@rfcbot reviewed

Comment on lines 12 to 13
program that is initialized with the initializer expression. This allocated object is disjoint from
all other allocated objects. All references and raw pointers to the static refer to the same
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to guarantee that they are disjoint from other allocated objects? Or just other statics. IE. if I have:

static FOO: i32 = 0;
const BAR: i32 = 0;

fn foo(){
    assert!(!core::ptr::eq(&FOO, &BAR));
}

Should we guarantee the assertion will always pass?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have concern permissions, but I think this should be addresses by T-opsem before the end of the FCP.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think we should guarantee that, and it's what the text already says, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

That prevents emitting unnamed_addr on const items.

Copy link
Member

Choose a reason for hiding this comment

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

unnamed_addr specifically does coalescing of non-significant-address-items into each other, and potentially into a significant-address item.

Copy link
Member

Choose a reason for hiding this comment

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

The LangRef says:

Note that a constant with significant address can be merged with a unnamed_addr constant, the result being a constant whose address is significant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that is unfortunate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think we should guarantee that, and it's what the text already says, isn't it?

That's what the proposed text says, my question is whether that's what we want it to say.

Copy link
Member

@workingjubilee workingjubilee Oct 21, 2024

Choose a reason for hiding this comment

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

I... honestly don't think that's surprising at all?

That's exactly what I would expect in a case of

const BIG_CONST: BigFrozen = BigFrozen::big_init();
static BIG_STATIC: BigFrozen = BIG_CONST;

That's the precise situation where the const will be unified "into" the static, and where it would be a clearly beneficial optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

I... honestly don't think that's surprising at all?

I guess we have different intuitions then.

But reality clearly works one way here, so I have updated the text to match reality. @chorman0773 please have a look.

@CAD97
Copy link

CAD97 commented Oct 20, 2024

@rfcbot reviewed

@workingjubilee
Copy link
Member

I have a concern: this proposed definition prevents emitting optimization annotations on non-static items that we may wish to have coalesced via optimizations that exploit the fact that const items have a non-significant address. Even if we wish to guarantee static unicity, it seems pointlessly penalizing to make this guarantee affect the ability to optimize, quite literally, the items that don't have unique addresses.

@saethlin
Copy link
Member

@rfcbot concern clarify-optimization-of-consts

I think the proposed text and the previous text have the same meaning, which is unfortunate because I think that we'd already specified that consts are not merged into statics. But given the participation on this PR, and its title which seems to be about overlap of statics, and the fact that I think rustc currently does not implement what is documented here, I would like the implications for consts to be spelled out clearly.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 25, 2024

But given the participation on this PR, and its title which seems to be about overlap of statics, and the fact that I think rustc currently does not implement what is documented here, I would like the implications for consts to be spelled out clearly.

I gave that a shot, please have a look.

@saethlin
Copy link
Member

@rustbot resolve clarify-optimization-of-consts

Copy link
Contributor

@chorman0773 chorman0773 left a comment

Choose a reason for hiding this comment

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

Given the concern is now resolved, I see no further issues with this.

@scottmcm
Copy link
Member

Do zero-sized local variables get an alloca, or what do we do about them?

Today they do, though I in fact have an open exploratory draft PR considering changing that:

https://github.com/rust-lang/rust/pull/132387/files#diff-160634de1c336f2cf325ff95b312777326f1ab29fec9b9b21d5ee9aae215ecf5R57-R67

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 14, 2024 via email

@nikic
Copy link
Contributor

nikic commented Nov 14, 2024

The LLVM issue is fixed by llvm/llvm-project#115728.

@traviscross
Copy link
Contributor

Given the change in 8dde3eb that excludes the challenging bit under discussion here with respect to ZSTs, let's...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 20, 2024

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

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@traviscross
Copy link
Contributor

Checking off the boxes for members of T-opsem as these were checked off in the earlier FCP.

@tmandry
Copy link
Member

tmandry commented Nov 20, 2024

I think the clarification proposed by @joshtriplett (or something more explicit) should be included, but otherwise I'm happy to go ahead with this.

@rfcbot reviewed

Co-authored-by: Josh Triplett <[email protected]>
@ehuss
Copy link
Contributor

ehuss commented Jan 9, 2025

@joshtriplett @nikomatsakis @scottmcm Do you have a chance to look at this and check your box or register a concern?

@rfcbot
Copy link

rfcbot commented Jan 9, 2025

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

psst @traviscross, I wasn't able to add the final-comment-period label, please do so.

Copy link
Contributor

@kpreid kpreid left a comment

Choose a reason for hiding this comment

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

I have a couple of thoughts on clarity and precision of the wording.

r[items.static.storage-disjointness]
If the `static` has a size of at least 1 byte, this allocated object is disjoint from all other such
`static` objects as well as heap-allocated and stack-allocated variables. However, the storage of
immutable `static` items can overlap with objects that do not themselves have a unique address, such
Copy link
Contributor

Choose a reason for hiding this comment

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

“Immutable static item” could be interpreted as meaning a static item without the mut keyword, but it must also exclude interior-mutable memory. Perhaps this should be clarified (or perhaps it doesn’t matter since this is a “can”, not “must”).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we should have this as a shared definition somewhere that we can reference.

src/items/static-items.md Outdated Show resolved Hide resolved
@rfcbot
Copy link

rfcbot commented Jan 19, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

psst @traviscross, I wasn't able to add the finished-final-comment-period label, please do so.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks @RalfJung!

@ehuss ehuss added this pull request to the merge queue Jan 25, 2025
Merged via the queue into rust-lang:master with commit 93b921c Jan 25, 2025
5 checks passed
Zalathar added a commit to Zalathar/rust that referenced this pull request Jan 28, 2025
Update books

## rust-lang/book

3 commits in 82a4a49789bc96db1a1b2a210b4c5ed7c9ef0c0d..fa312a343fbff01bc6cef393e326817f70719813
2025-01-22 17:14:29 UTC to 2025-01-22 15:09:26 UTC

- chore: reformat src with dprint (rust-lang/book#4211)
- Redirects: get rid of the weird gap in Ch. 20 sections! (rust-lang/book#4209)
- Document that `use` is also for `precise capturing` (rust-lang/book#4210)

## rust-lang/edition-guide

1 commits in d56e0f3a0656b7702ca466d4b191e16c28262b82..4ed5a1a4a2a7ecc2e529a5baaef04f7bc7917eda
2025-01-21 21:39:56 UTC to 2025-01-21 21:39:56 UTC

- Add alternatives for static-mut-refs (rust-lang/edition-guide#354)

## rust-lang/nomicon

3 commits in 625b200e5b33a5af35589db0bc454203a3d46d20..bc2298865544695c63454fc1f9f98a3dc22e9948
2025-01-23 19:01:24 UTC to 2025-01-20 14:37:52 UTC

- corrected grammatical error. (rust-lang/nomicon#477)
- Remove `#![start]` attribute (rust-lang/nomicon#478)
- Update guidance on uninitialized fields to use &raw mut instead of addr_of_mut! (rust-lang/nomicon#476)

## rust-lang/reference

10 commits in 293af991003772bdccf2d6b980182d84dd055942..93b921c7d3213d38d920f7f905a3bec093d2217d
2025-01-25 21:59:01 UTC to 2025-01-14 17:28:04 UTC

- distinct 'static' items never overlap (rust-lang/reference#1657)
- Change `'_static` to `'static` as an invalid lifetime parameter name (rust-lang/reference#1721)
- reword reference about inert attributes (rust-lang/reference#1719)
- Provide a better error message for broken links in mdbook-spec (rust-lang/reference#1716)
- Remove unstable vectorcall (rust-lang/reference#1717)
- Move the function pointer example (rust-lang/reference#1718)
- references and Box must be non-null (rust-lang/reference#1715)
- Fix filename for theme customization (rust-lang/reference#1711)
- Add Identifier Syntax to Several Chapters (rust-lang/reference#1597)
- move r[rules] to the left of the main body, using a grid (rust-lang/reference#1710)
Zalathar added a commit to Zalathar/rust that referenced this pull request Jan 28, 2025
Update books

## rust-lang/book

3 commits in 82a4a49789bc96db1a1b2a210b4c5ed7c9ef0c0d..fa312a343fbff01bc6cef393e326817f70719813
2025-01-22 17:14:29 UTC to 2025-01-22 15:09:26 UTC

- chore: reformat src with dprint (rust-lang/book#4211)
- Redirects: get rid of the weird gap in Ch. 20 sections! (rust-lang/book#4209)
- Document that `use` is also for `precise capturing` (rust-lang/book#4210)

## rust-lang/edition-guide

1 commits in d56e0f3a0656b7702ca466d4b191e16c28262b82..4ed5a1a4a2a7ecc2e529a5baaef04f7bc7917eda
2025-01-21 21:39:56 UTC to 2025-01-21 21:39:56 UTC

- Add alternatives for static-mut-refs (rust-lang/edition-guide#354)

## rust-lang/nomicon

3 commits in 625b200e5b33a5af35589db0bc454203a3d46d20..bc2298865544695c63454fc1f9f98a3dc22e9948
2025-01-23 19:01:24 UTC to 2025-01-20 14:37:52 UTC

- corrected grammatical error. (rust-lang/nomicon#477)
- Remove `#![start]` attribute (rust-lang/nomicon#478)
- Update guidance on uninitialized fields to use &raw mut instead of addr_of_mut! (rust-lang/nomicon#476)

## rust-lang/reference

10 commits in 293af991003772bdccf2d6b980182d84dd055942..93b921c7d3213d38d920f7f905a3bec093d2217d
2025-01-25 21:59:01 UTC to 2025-01-14 17:28:04 UTC

- distinct 'static' items never overlap (rust-lang/reference#1657)
- Change `'_static` to `'static` as an invalid lifetime parameter name (rust-lang/reference#1721)
- reword reference about inert attributes (rust-lang/reference#1719)
- Provide a better error message for broken links in mdbook-spec (rust-lang/reference#1716)
- Remove unstable vectorcall (rust-lang/reference#1717)
- Move the function pointer example (rust-lang/reference#1718)
- references and Box must be non-null (rust-lang/reference#1715)
- Fix filename for theme customization (rust-lang/reference#1711)
- Add Identifier Syntax to Several Chapters (rust-lang/reference#1597)
- move r[rules] to the left of the main body, using a grid (rust-lang/reference#1710)
Zalathar added a commit to Zalathar/rust that referenced this pull request Jan 29, 2025
Update books

## rust-lang/book

3 commits in 82a4a49789bc96db1a1b2a210b4c5ed7c9ef0c0d..fa312a343fbff01bc6cef393e326817f70719813
2025-01-22 17:14:29 UTC to 2025-01-22 15:09:26 UTC

- chore: reformat src with dprint (rust-lang/book#4211)
- Redirects: get rid of the weird gap in Ch. 20 sections! (rust-lang/book#4209)
- Document that `use` is also for `precise capturing` (rust-lang/book#4210)

## rust-lang/edition-guide

1 commits in d56e0f3a0656b7702ca466d4b191e16c28262b82..4ed5a1a4a2a7ecc2e529a5baaef04f7bc7917eda
2025-01-21 21:39:56 UTC to 2025-01-21 21:39:56 UTC

- Add alternatives for static-mut-refs (rust-lang/edition-guide#354)

## rust-lang/nomicon

3 commits in 625b200e5b33a5af35589db0bc454203a3d46d20..bc2298865544695c63454fc1f9f98a3dc22e9948
2025-01-23 19:01:24 UTC to 2025-01-20 14:37:52 UTC

- corrected grammatical error. (rust-lang/nomicon#477)
- Remove `#![start]` attribute (rust-lang/nomicon#478)
- Update guidance on uninitialized fields to use &raw mut instead of addr_of_mut! (rust-lang/nomicon#476)

## rust-lang/reference

10 commits in 293af991003772bdccf2d6b980182d84dd055942..93b921c7d3213d38d920f7f905a3bec093d2217d
2025-01-25 21:59:01 UTC to 2025-01-14 17:28:04 UTC

- distinct 'static' items never overlap (rust-lang/reference#1657)
- Change `'_static` to `'static` as an invalid lifetime parameter name (rust-lang/reference#1721)
- reword reference about inert attributes (rust-lang/reference#1719)
- Provide a better error message for broken links in mdbook-spec (rust-lang/reference#1716)
- Remove unstable vectorcall (rust-lang/reference#1717)
- Move the function pointer example (rust-lang/reference#1718)
- references and Box must be non-null (rust-lang/reference#1715)
- Fix filename for theme customization (rust-lang/reference#1711)
- Add Identifier Syntax to Several Chapters (rust-lang/reference#1597)
- move r[rules] to the left of the main body, using a grid (rust-lang/reference#1710)
fmease added a commit to fmease/rust that referenced this pull request Jan 29, 2025
Update books

## rust-lang/book

3 commits in 82a4a49789bc96db1a1b2a210b4c5ed7c9ef0c0d..fa312a343fbff01bc6cef393e326817f70719813
2025-01-22 17:14:29 UTC to 2025-01-22 15:09:26 UTC

- chore: reformat src with dprint (rust-lang/book#4211)
- Redirects: get rid of the weird gap in Ch. 20 sections! (rust-lang/book#4209)
- Document that `use` is also for `precise capturing` (rust-lang/book#4210)

## rust-lang/edition-guide

1 commits in d56e0f3a0656b7702ca466d4b191e16c28262b82..4ed5a1a4a2a7ecc2e529a5baaef04f7bc7917eda
2025-01-21 21:39:56 UTC to 2025-01-21 21:39:56 UTC

- Add alternatives for static-mut-refs (rust-lang/edition-guide#354)

## rust-lang/nomicon

3 commits in 625b200e5b33a5af35589db0bc454203a3d46d20..bc2298865544695c63454fc1f9f98a3dc22e9948
2025-01-23 19:01:24 UTC to 2025-01-20 14:37:52 UTC

- corrected grammatical error. (rust-lang/nomicon#477)
- Remove `#![start]` attribute (rust-lang/nomicon#478)
- Update guidance on uninitialized fields to use &raw mut instead of addr_of_mut! (rust-lang/nomicon#476)

## rust-lang/reference

10 commits in 293af991003772bdccf2d6b980182d84dd055942..93b921c7d3213d38d920f7f905a3bec093d2217d
2025-01-25 21:59:01 UTC to 2025-01-14 17:28:04 UTC

- distinct 'static' items never overlap (rust-lang/reference#1657)
- Change `'_static` to `'static` as an invalid lifetime parameter name (rust-lang/reference#1721)
- reword reference about inert attributes (rust-lang/reference#1719)
- Provide a better error message for broken links in mdbook-spec (rust-lang/reference#1716)
- Remove unstable vectorcall (rust-lang/reference#1717)
- Move the function pointer example (rust-lang/reference#1718)
- references and Box must be non-null (rust-lang/reference#1715)
- Fix filename for theme customization (rust-lang/reference#1711)
- Add Identifier Syntax to Several Chapters (rust-lang/reference#1597)
- move r[rules] to the left of the main body, using a grid (rust-lang/reference#1710)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2025
Rollup merge of rust-lang#136143 - rustbot:docs-update, r=ehuss

Update books

## rust-lang/book

3 commits in 82a4a49789bc96db1a1b2a210b4c5ed7c9ef0c0d..fa312a343fbff01bc6cef393e326817f70719813
2025-01-22 17:14:29 UTC to 2025-01-22 15:09:26 UTC

- chore: reformat src with dprint (rust-lang/book#4211)
- Redirects: get rid of the weird gap in Ch. 20 sections! (rust-lang/book#4209)
- Document that `use` is also for `precise capturing` (rust-lang/book#4210)

## rust-lang/edition-guide

1 commits in d56e0f3a0656b7702ca466d4b191e16c28262b82..4ed5a1a4a2a7ecc2e529a5baaef04f7bc7917eda
2025-01-21 21:39:56 UTC to 2025-01-21 21:39:56 UTC

- Add alternatives for static-mut-refs (rust-lang/edition-guide#354)

## rust-lang/nomicon

3 commits in 625b200e5b33a5af35589db0bc454203a3d46d20..bc2298865544695c63454fc1f9f98a3dc22e9948
2025-01-23 19:01:24 UTC to 2025-01-20 14:37:52 UTC

- corrected grammatical error. (rust-lang/nomicon#477)
- Remove `#![start]` attribute (rust-lang/nomicon#478)
- Update guidance on uninitialized fields to use &raw mut instead of addr_of_mut! (rust-lang/nomicon#476)

## rust-lang/reference

10 commits in 293af991003772bdccf2d6b980182d84dd055942..93b921c7d3213d38d920f7f905a3bec093d2217d
2025-01-25 21:59:01 UTC to 2025-01-14 17:28:04 UTC

- distinct 'static' items never overlap (rust-lang/reference#1657)
- Change `'_static` to `'static` as an invalid lifetime parameter name (rust-lang/reference#1721)
- reword reference about inert attributes (rust-lang/reference#1719)
- Provide a better error message for broken links in mdbook-spec (rust-lang/reference#1716)
- Remove unstable vectorcall (rust-lang/reference#1717)
- Move the function pointer example (rust-lang/reference#1718)
- references and Box must be non-null (rust-lang/reference#1715)
- Fix filename for theme customization (rust-lang/reference#1711)
- Add Identifier Syntax to Several Chapters (rust-lang/reference#1597)
- move r[rules] to the left of the main body, using a grid (rust-lang/reference#1710)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.