Replace mem::uninitialized with mem::MaybeUninit #165

LiHRaM · 2019-09-18T14:35:12Z

closes #164

I ran some examples, and everything seemed to work just the same. Using ::zeroed, I believe the structs in these cases can handle zero values for all of their fields. I am not familiar with the external functions, however, and don't know if we want to assume_init after calling them.

druid-shell/src/windows/mod.rs

cmyr · 2019-09-18T14:52:30Z

druid-shell/src/windows/mod.rs

@@ -391,7 +391,7 @@ impl WndProc for MyWndProc {
                if let Ok(mut s) = self.state.try_borrow_mut() {
                    let s = s.as_mut().unwrap();
                    if s.dcomp_state.is_some() {
-                        let mut rect: RECT = mem::uninitialized();


This stuff is new to me too, but reading the docs for MaybeUninit it doesn't sound like assume_init is what we want. I think we want to just use uninit(), and then call assum_init after we've called the external function.

Basically what's happening in all these places is we're passing a memory location to the windows API, which is supposed to write something to that location; so the memory is not safe to access until after that write has happened (and I imagine all of these winapi functions return a flag indicating if there is an error or not, and if there's an error we should also assume the memory is uninitialized...)

Yeah, so the current refactoring initializes the memory to safe values and assumes init, which I guess is only slightly safer than the previous use of mem::uninitialized - I was just treading the path of least resistance there. The problem then becomes whether passing MaybeUninit to these external functions is the same as passing the pointers themselves, or whether we need to do some black magic there. 🤔I wasn't able to tell from the docs how MaybeUninit translates externally.

ah okay, I guess the main question I'd have is whether zeros is valid data for all the types used here? If it is then that works for me!

and yes, passing a pointer to the MaybeUninit is the same as passing a pointer to the value; all MaybeUninit does is use the type system to force us to be explicit about initialization on the Rust side.

This is fancier than needed for RECT, you can just do mem::zeroed instead of mem::uninitialized. It's a tiny optimization not to zero, but when I wrote the code I was doing things "the C way."

Okay, good to know. Now my only concern is with the implications of GetClientRect returning false, is that something we want to log and/or return an error?

Yes. This is extremely unlikely, but we want to program defensively in any case.

Thanks for the feedback, friends! It's updated now. 😄

LiHRaM · 2019-09-18T15:25:10Z

druid-shell/src/windows/win_main.rs

@@ -84,7 +84,7 @@ impl RunLoop {

                // Handle windows messages
                loop {
-                    let mut msg = mem::uninitialized();
+                    let mut msg = mem::MaybeUninit::<MSG>::zeroed().assume_init();


@cmyr Am unable to find MSG, which is supposed to be the type for msg here.

RECT should be fine to zero, as it just contains four numerical values.
DXGI_ADAPTER_DESC is confusing to me. The type declaration is as follows:

struct DXGI_ADAPTER_DESC { Description: [WCHAR; 128], VendorId: UINT, DeviceId: UINT, SubSysId: UINT, Revision: UINT, DedicatedVideoMemory: SIZE_T, DedicatedSystemMemory: SIZE_T, SharedSystemMemory: SIZE_T, AdapterLuid: LUID, }

My search-foo fails me here, am unable to find out what LUID is. On the other hand, I assume that Rust initializes the vector for Description within the struct instead of using a pointer to an external vector, which would make zeroing it safe.

raphlinus · 2019-09-18T15:48:44Z

druid-shell/src/windows/mod.rs

@@ -391,7 +391,7 @@ impl WndProc for MyWndProc {
                if let Ok(mut s) = self.state.try_borrow_mut() {
                    let s = s.as_mut().unwrap();
                    if s.dcomp_state.is_some() {
-                        let mut rect: RECT = mem::uninitialized();


This is fancier than needed for RECT, you can just do mem::zeroed instead of mem::uninitialized. It's a tiny optimization not to zero, but when I wrote the code I was doing things "the C way."

raphlinus · 2019-09-18T15:52:43Z

druid-shell/src/windows/mod.rs

@@ -899,7 +899,7 @@ unsafe fn choose_adapter(factory: *mut IDXGIFactory2) -> *mut IDXGIAdapter {
        if !SUCCEEDED((*factory).EnumAdapters(i, &mut adapter)) {
            break;
        }
-        let mut desc: DXGI_ADAPTER_DESC = mem::uninitialized();
+        let mut desc = mem::MaybeUninit::<DXGI_ADAPTER_DESC>::zeroed().assume_init();


I think the way I'd write this is:

let desc = mem::MaybeUninit::uninit(); let hr = (*adapter).GetDesc(desc.as_mut_ptr()); ...check hr... let desc = desc.assume_init();

I'm noticing that the existing code is not checking the return code. This is UB if the call fails.

raphlinus

See inline about wide string manipulation; maybe a separate issue? Thanks for this!

raphlinus · 2019-09-19T16:41:02Z

druid-shell/src/windows/mod.rs

+            error!("Failed to get adapter description: {:?}", Error::Hr(hr));
+            break;
+        }
+        let mut desc: DXGI_ADAPTER_DESC = desc.assume_init();


It feels like this doesn't really need to be mut. The problem is that our "util" module takes a LPWSTR, which is more idiomatically C. I think the solution is to use *const u16 and the from_wide_ptr_null() method from wio and remove our hand-rolled "util" stuff, but that's arguably a separate issue from the scope of this PR, which is specifically about uninit.

Sounds good. I'll add an issue for it.

LiHRaM commented Sep 18, 2019

View reviewed changes

druid-shell/src/windows/mod.rs Outdated Show resolved Hide resolved

cmyr reviewed Sep 18, 2019

View reviewed changes

LiHRaM commented Sep 18, 2019

View reviewed changes

raphlinus reviewed Sep 18, 2019

View reviewed changes

LiHRaM force-pushed the maybeuninit branch from 3b33416 to 27c8de9 Compare September 19, 2019 16:29

LiHRaM added 2 commits September 19, 2019 18:29

Replace mem::uninitialized with mem::MaybeUninit

e420d0b

check responses, be less verbose

a6e2fbd

LiHRaM force-pushed the maybeuninit branch from 27c8de9 to a6e2fbd Compare September 19, 2019 16:29

raphlinus approved these changes Sep 19, 2019

View reviewed changes

LiHRaM merged commit 4e61d0c into master Sep 19, 2019

LiHRaM deleted the maybeuninit branch September 19, 2019 16:55

LiHRaM mentioned this pull request Sep 19, 2019

Replace util ToWide and FromWide functions with wio functions #173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace mem::uninitialized with mem::MaybeUninit #165

Replace mem::uninitialized with mem::MaybeUninit #165

LiHRaM commented Sep 18, 2019

cmyr Sep 18, 2019

LiHRaM Sep 18, 2019 •

edited

Loading

cmyr Sep 18, 2019

cmyr Sep 18, 2019

raphlinus Sep 18, 2019

LiHRaM Sep 18, 2019

raphlinus Sep 18, 2019

LiHRaM Sep 19, 2019

LiHRaM Sep 18, 2019

raphlinus Sep 18, 2019

raphlinus Sep 18, 2019

raphlinus left a comment

raphlinus Sep 19, 2019 •

edited

Loading

LiHRaM Sep 19, 2019

Replace mem::uninitialized with mem::MaybeUninit #165

Replace mem::uninitialized with mem::MaybeUninit #165

Conversation

LiHRaM commented Sep 18, 2019

Choose a reason for hiding this comment

LiHRaM Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus left a comment

Choose a reason for hiding this comment

raphlinus Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHRaM Sep 18, 2019 •

edited

Loading

raphlinus Sep 19, 2019 •

edited

Loading