Skip to content

Add container_of and offset_of macros. #158

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

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

wedsonaf
Copy link

@wedsonaf wedsonaf commented Apr 1, 2021

No description provided.

Copy link

@jabedude jabedude left a comment

Choose a reason for hiding this comment

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

Looks great, one small suggestion below:

let tmp = core::mem::MaybeUninit::<$type>::uninit();
let ptr = tmp.as_ptr();
#[allow(unused_unsafe)]
let dev = unsafe { core::ptr::addr_of!((*ptr).$($f)*) as *const u8 };
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, *ptr is OK because dereferencing a pointer is only UB if dangling or unaligned, but in our case it is just not initialized; and it does not count as a memory read either. If this is correct, can we put it in a SAFETY: ... comment pretty please?

(Someone that has followed the entire discussion on the &raw RFC and related bits can tell us whether this is the case or not -- but one way or the other, it works in practice today, Miri does not complain, and hopefully core provides this macro soon; so it is OK for me).

Nit: we can reduce the scope a bit:

Suggested change
let dev = unsafe { core::ptr::addr_of!((*ptr).$($f)*) as *const u8 };
let dev = unsafe { core::ptr::addr_of!((*ptr).$($f)*) } as *const u8;

Copy link
Author

@wedsonaf wedsonaf Apr 1, 2021

Choose a reason for hiding this comment

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

IIUC, *ptr is OK because dereferencing a pointer is only UB if dangling or unaligned, but in our case it is just not initialized; and it does not count as a memory read either. If this is correct, can we put it in a SAFETY: ... comment pretty please?

Creating a reference -- you don't even need to read or write it -- to a uninitialised memory is considered UB. This is reason for the existence of addr_of. See details here: https://doc.rust-lang.org/nightly/core/ptr/macro.addr_of.html

Nit: we can reduce the scope a bit:

Done.

Copy link
Member

@ojeda ojeda Apr 1, 2021

Choose a reason for hiding this comment

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

Creating a reference -- you don't even need to read or write it -- to a uninitialised memory is considered UB. This is reason for the existence of addr_of. See details here: https://doc.rust-lang.org/nightly/core/ptr/macro.addr_of.html

Not sure what do you mean (I said pointer, not reference -- that is why it is safe, and the reason why &raw was introduced AFAIUI, which is what addr_of! uses internally).

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what do you mean (I said pointer, not reference -- that is why it is safe, and the reason why &raw was introduced AFAIUI, which is what addr_of! uses internally).

Suppose you have a pointer to a struct Test from the example. If you want to get a pointer to Test::b from that, you first need to create a reference to Test, then access field b, and take a pointer to that. This intermediate step leads to UB because we created a reference to uninitialised data. &raw helps us avoid this.

Copy link
Member

@ojeda ojeda Apr 1, 2021

Choose a reason for hiding this comment

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

Yes, but I never said otherwise -- I was talking about *ptr in the part you quoted of the original message.

Let me try to clarify: addr_of! (&raw) is not unsafe on its own. What I was discussing was the dereference on ptr which is what requires the unsafe block. So I explained that such a dereference is valid because the pointer isn't dangling nor unaligned (general requirements on dereferencing a pointer) and because we don't actually read from it (requirement on reads on pointers to uninitialized data).

Copy link
Member

Choose a reason for hiding this comment

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

If the last edits don't clarify what I am saying, let's have a quick meeting -- I don't know what else to say to clarify this more.

Copy link
Author

Choose a reason for hiding this comment

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

I understand what you're saying now.

What I am saying now is that we don't need to justify this dereference because it isn't a real dereference. In C parlance, here's what I'm trying to convey: when we have &ptr->field, this gets desugared to &(*ptr).field, and neither of which is a real deference, they both get compiled to (ignoring types, considering ptr a uintptr_t)ptr+K, where K is the offset of the field.

The code above gets expanded to &raw (*ptr).field, which isn't a real dereference either. So I mean it literally when I say that we don't actually dereference it.

Does it make sense?

Copy link
Member

@ojeda ojeda Apr 1, 2021

Choose a reason for hiding this comment

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

it isn't a real dereference. In C parlance, here's what I'm trying to convey: (...)

Yeah, that is what I meant by just a GEP in LLVM IR parlance, i.e. "compute the address", without any "runtime dereference" -- we agree on this.

What I am saying now is that we don't need to justify this dereference

Even if it is not a "runtime dereference", it is still a "Rust dereference", and for those, there are two requirements as you know (not dangling + aligned).

These two are still required even if one is using &raw, i.e. the RFC for it explains it only covers the problem of creating an intermediate reference, but it still means the pointer needs to point somewhere valid and, in fact, land somewhere in the same allocation; e.g. the RFC claims this is UB even though x is properly aligned and non-null:

let x: *mut Struct = NonNull::dangling().as_ptr();
let field: *mut Field = &raw mut (*x).field;

I checked the generated code for our case, and indeed the GEP that gets generated (with the latest nightly, at least) is inbounds.

So we need to justify the dereference in all cases, at least today (this might change later in Rust -- but that is why I said I am OK with the implementation for the moment, since in practice it works today).

Then, the second part is that since the pointer comes from MaybeUninit::as_ptr(), we have an additional requirement which is that the pointer is never read from -- but just dereferencing inside the &raw seems to be OK (as in "Rust dereference", not "runtime dereference").

I actually don't know how this second part is properly justified in Rust terms even when using &raw (and from a quick look, people seem to be discussing this one still), but checking the LLVM IR indeed does not perform a load today, and just a GEP (inbounds, but that is OK for us since we do have an object), so again, I am OK with the implementation for the moment.

Finally, here is where I suggested improving the wording on "actually dereference" by avoiding the word "dereference" altogether -- instead saying what we actually need to avoid UB on this second part, which is reading/loading from the pointer. This also solves the issue of talking about "dereference" to mean the actual load ("runtime dereference") from the GEP ("Rust dereference" inside a &raw).

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. I'll update the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! It was not a big deal anyway, I was more concerned about being in the same page regarding the implementation and the requirements. I will merge this as soon as you do it.

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

(Waiting for @ojeda to have another look before merge)

@ojeda ojeda merged commit de33f8a into Rust-for-Linux:rust Apr 1, 2021
@wedsonaf wedsonaf deleted the container_of branch April 1, 2021 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants