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

Add offset_of! macro (RFC 3308) #106934

Merged
merged 10 commits into from
Apr 22, 2023
Merged

Add offset_of! macro (RFC 3308) #106934

merged 10 commits into from
Apr 22, 2023

Conversation

beepster4096
Copy link
Contributor

Implements rust-lang/rfcs#3308 (tracking issue #106655) by adding the built in macro core::mem::offset_of. Two of the future possibilities are also implemented:

  • Nested field accesses (without array indexing)
  • DST support (for Sized fields)

I wrote this a few months ago, before the RFC merged. Now that it's merged, I decided to rebase and finish it.

cc @thomcc (RFC author)

@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

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

rustbot commented Jan 16, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occured in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@est31
Copy link
Member

est31 commented Jan 17, 2023

What a great PR. It also adds DST support from the get-go which is amazing (I care about that use case a bit xD).

I've seen tests for nested fields, but couldn't find any tests for DSTs. Maybe something like this:

#[test]
#[cfg(not(bootstrap))]
fn offset_in_dst() {
    #[repr(C)]
    struct Foo {
        x: u8,
        y: u16,
        more: [u8],
    }

    const X_OFFSET: usize = offset_of!(Foo, x);
    const Y_OFFSET: usize = offset_of!(Foo, y);

    assert_eq!(X_OFFSET, 0);
    assert_eq!(Y_OFFSET, 2);
}

The implementation also errors nicely in the case of offset_of!(Foo, unsized_field). It would be nice to have that as a test.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Jan 18, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in src/tools/cargo

cc @ehuss

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Frontend LGTM, but someone else needs to look at MIR, const eval and codegen.
r? rust-lang/compiler

@rustbot rustbot assigned cjgillot and unassigned petrochenkov Jan 19, 2023
@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

cjgillot commented Jan 21, 2023

Could you add a few tests?

  1. That we give a sensible error on offset_of!(type) and offset_of!(type, ).

  2. That the value returned by offset_of! is correct compared to directly taking the field address for a few relevant combinations of fields. Something like this: edit: just saw you put them in coretests.

assert_eq!(addr_of!(base).addr() + offset_of!(base_ty, field), addr_of!(base.field));
  1. As you implemented in rustc_const_eval, a mir-opt test that ConstProp replaces with the correct value when the type is sufficiently known. One test on a non-generic type, one on a partially generic with known layout, and one on a partially generic with unknown layout.

@cjgillot cjgillot 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 Jan 21, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

Thank you for the work you've put into this, @drmeepster! I think this is ready to be merged now, congrats!

@WaffleLapkin
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2023

📌 Commit 99abe44 has been approved by WaffleLapkin

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 Apr 21, 2023
@bors
Copy link
Contributor

bors commented Apr 22, 2023

⌛ Testing commit 99abe44 with merge 80a2ec4...

@bors
Copy link
Contributor

bors commented Apr 22, 2023

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 80a2ec4 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (80a2ec4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.7% [1.7%, 1.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.6% [-4.8%, -2.7%] 3
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Apr 29, 2023
Add offset_of! macro (RFC 3308)

Implements rust-lang/rfcs#3308 (tracking issue rust-lang#106655) by adding the built in macro `core::mem::offset_of`. Two of the future possibilities are also implemented:

* Nested field accesses (without array indexing)
* DST support (for `Sized` fields)

I wrote this a few months ago, before the RFC merged. Now that it's merged, I decided to rebase and finish it.

cc `@thomcc` (RFC author)
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 5, 2023
Add offset_of! macro (RFC 3308)

Implements rust-lang/rfcs#3308 (tracking issue rust-lang#106655) by adding the built in macro `core::mem::offset_of`. Two of the future possibilities are also implemented:

* Nested field accesses (without array indexing)
* DST support (for `Sized` fields)

I wrote this a few months ago, before the RFC merged. Now that it's merged, I decided to rebase and finish it.

cc `@thomcc` (RFC author)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 9, 2023
Implement builtin # syntax and use it for offset_of!(...)

Add `builtin #` syntax to the parser, as well as a generic infrastructure to support both item and expression position builtin syntaxes. The PR also uses this infrastructure for the implementation of the `offset_of!` macro, added by rust-lang#106934.

cc `@petrochenkov` `@DrMeepster`

cc rust-lang#110680 `builtin #` tracking issue
cc rust-lang#106655 `offset_of!` tracking issue
@est31 est31 mentioned this pull request May 22, 2023
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.