Bug: D3D11 XXSet* functions incorrectly project optional array of optional objects as optional array of required objects

[chyyran]

I'll use PSSetShaderResources as an example but this applies to most XXSet* functions.

PSSetShaderResources has the following signature

void PSSetShaderResources(
  [in]           UINT                     StartSlot,
  [in]           UINT                     NumViews,
  [in, optional] ID3D11ShaderResourceView * const *ppShaderResourceViews
);

In 0.43 this was projected as

unsafe fn PSSetShaderResources(&self, startslot: u32, ppshaderresourceviews: Option<&[Option<ID3D11ShaderResourceView>]>)

This signatured allowed Rust to express the following C++ pattern

auto shaderResources = {
   resource1,
   nullptr,
   resource2
};

context.PSSetShaderResources(0, &shaderResources);

let shader_resources: &[Option<ID3D11ShaderResourceView>; 3] = &[
   Some(resource1),
   None,
   Some(resource2)
];

context.PSSetShaderResources(0, Some(shader_resources));

This pattern is inexpressible in 0.44 without transmute because PSSetShaderResources is projected as

unsafe fn PSSetShaderResources(&self, startslot: u32, ppshaderresourceviews: ::core::option::Option<&[ID3D11ShaderResourceView]>)

i.e. every ID3D11ShaderResourceView must be valid.

The workaround is to use the following transmute

let shader_resources: &[Option<ID3D11ShaderResourceView>; 3] = &[
   Some(resource1),
   None,
   Some(resource2)
];

// windows-rs does not guarantee that the layout of IUnknown will remain NonNull<c_void>. If it changes, the following 
// transmute goes from UB that "works on my machine", to instant nasal demons. This const assert should give us the 
// confidence that the transmute "does what it should".

// This also assumes that all D3D11 COM pointer types wrap an IUnknown. I feel like this is a stronger guarantee than the
// Rust layout of IUnknown given COM lite pointers are guaranteed castable to IUnknown.

const _: () = assert!(std::mem::size_of::<Option<windows::core::IUnknown>>() == std::mem::size_of::<windows::core::IUnknown>());

context.PSSetShaderResources(0, Some(std::mem::transmute(shader_resources.as_ref())));

Which, given that IUnknown is NonNull<c_void>, smells like UB to me, even if it "works".

[chyyran]

I think this bug was introduced in #2233

[chyyran]

Also related: #2105

While it may seem to make sense passing nulls in if you know your inputs are non-null beforehand, it's certainly possible in a shader (and not UB) to have some values bound and some values unbound accidentally if you don't. My use care requires that I don't know what resources can be bound beforehand, so some of them can be null.

The new projection makes it impossible to unset certain resources. While OMSetRenderTargets unbinds everything on a single nullptr, PSSetShaderResources makes no such guarantee.

As a side note, it is UB to transmute &[Option<NonNull<T>>] to &[NonNull<T>] (because now the non-null invariant is violated if it is ever observed), but not UB to transmute from &[NonNull<T>] to &[Option<NonNull<T>>], since Option guarantees the niche optimization they have the same layout. I'd like to reiterate my suggestion in #1339 (comment) to experiment with IntoParam or similar to allow such methods to take both &[NonNull<T>] and &[Option<NonNull<T>>]

[kennykerr]

This was specifically requested to improve usability (#2233).

@damyanp / @robmikh is there a reason we need to support null entries in such arrays?

[chyyran]

Without null entries it's impossible to unbind shader resources without invoking UB via violating the NonNull invariant of IUnknown.

[kennykerr]

Fair enough - I can revert this behavior. Is there a minimal standalone repro I can use to add a permanent test to ensure this behavior is preserved?

[chyyran]

I have a DX11 hello triangle based off the DX12 example in this repo here (for windows 0.43.0 currently, I've ported it to windows 0.44 but that version includes some extraneous things for my own use)

https://github.com/SnowflakePowered/librashader/blob/5d476d5229f2b53f76ac227b63837f22a7faf18a/librashader-runtime-dx11/src/hello_triangle.rs

It probably needs some cleaning up for which I don't immediately have time to attend to right now but as a base it should be OK to work with.

---

The repository is MPL2/GPL3, but I hereby release the contents of
https://github.com/SnowflakePowered/librashader/blob/5d476d5229f2b53f76ac227b63837f22a7faf18a/librashader-runtime-dx11/src/hello_triangle.rs under the MIT license.

[kennykerr]

Thanks, I'm really just looking for a single API call to use as part of automated testing. Not a complete sample. Not to worry, I'll dig around for one. 😊

[kennykerr]

fyi @Muon