Add container_of and offset_of macros.#158
Conversation
jabedude
left a comment
There was a problem hiding this comment.
Looks great, one small suggestion below:
rust/kernel/lib.rs
Outdated
| 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 }; |
There was a problem hiding this comment.
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:
| let dev = unsafe { core::ptr::addr_of!((*ptr).$($f)*) as *const u8 }; | |
| let dev = unsafe { core::ptr::addr_of!((*ptr).$($f)*) } as *const u8; |
There was a problem hiding this comment.
IIUC,
*ptris 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 aSAFETY: ...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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Not sure what do you mean (I said pointer, not reference -- that is why it is safe, and the reason why
&rawwas introduced AFAIUI, which is whataddr_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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Fair enough. I'll update the comment.
There was a problem hiding this comment.
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.
No description provided.