Bug: ID3D12InfoQueue1::RegisterMessageCallback context pointer type does not match D3D12MessageFunc

[david7a68]

Which crate is this about?

windows

Crate version

0.48.0

Summary

RegisterMessageCallback takes as its pcontext pointer a *const c_void. However, D3D12MessageFunc takes a *mut c_void.

Toolchain version/configuration

Default host: x86_64-pc-windows-msvc
rustup home: C:\Users\David.rustup

stable-x86_64-pc-windows-msvc (default)
rustc 1.70.0 (90c541806 2023-05-31)

Reproducible example

No response

Crate manifest

No response

Expected behavior

No response

Actual behavior

No response

Additional comments

Thank you for your hard work!

The correct behavior is unclear from context. However, it seems it should probably be const since the callback can be called from multiple threads a la this document.

Edit: Fix links

[kennykerr]

Thanks for reporting! This looks like a discrepancy in the Win32 metadata. I'll forward this issue for consideration.

[mikebattista]

@kennykerr could you clarify what metadata change is required here?

[riverar]

@damyanp @MarijnS95 Heads up, this typedef is missing annotations in d3d12sdklayers.idl

https://github.com/microsoft/DirectX-Headers/blob/master/include/directx/d3d12sdklayers.idl#L1750-L1754

typedef void (__stdcall *D3D12MessageFunc) (D3D12_MESSAGE_CATEGORY Category,
    D3D12_MESSAGE_SEVERITY Severity,
    D3D12_MESSAGE_ID ID,
    LPCSTR pDescription,
    void* pContext);

...

HRESULT RegisterMessageCallback(
    [annotation("_In_")]    D3D12MessageFunc CallbackFunc, 
    [annotation("_In_")]    D3D12_MESSAGE_CALLBACK_FLAGS CallbackFilterFlags,
    [annotation("_In_")]    void* pContext, 
    [annotation("_Inout_")] DWORD *pCallbackCookie);

@mikebattista Shall we wait for the DirectX folks to fixup the IDL? Or fix it here and upstream it?

[riverar]

I'd prefer blocking on upstream but here's a branch if you need (rafael/d3d12_const_ctx)

[damyanp]

I'm trying to figure out what the right fix is (sorry, I'm having to page in Rust, D3D12 and SAL at the moment!)

The callback should be allowed to write to the memory pContext points at. Semi-reasonable implementation of the callback might be something like:

void MyMessageFunc(D3D12_MESSAGE_CATEGORY Category,
    D3D12_MESSAGE_SEVERITY Severity,
    D3D12_MESSAGE_ID ID,
    LPCSTR pDescription,
    void* pContext)
{
    MessageContainer* messages = reinterpret_cast<MessageContainer*>(pContext);
    message->Add(Category, Severity, ID, pDescription);
}

Wouldn't this mean we need pContext to be InOut?

[riverar]

Assuming those annotations are supposed to be SAL markings, the _in_ use here indicates "input parameter to a function, unmodified by called function".

void Foo(..., _in_ void* pContext)
{
  *pContext = 42; // Bad
}

I don't think _in_ or _inout_ extend beyond describing the read/write characteristics of the parameter itself. That is, whether or not pContext mutates internally indirectly via methods is unknown, with these elementary markings at least. Looking around various other headers, that seems to track but don't bet the farm on it. @oldnewthing thoughts?

msxml6.h getURI https://learn.microsoft.com/previous-versions/windows/desktop/ms762697(v=vs.85) contains an _in_ IXMLDOMNode* pContext parameter, yet IXMLDOMNode has read/write methods.

dbgmodel.h IModelMethod https://learn.microsoft.com/windows-hardware/drivers/ddi/dbgmodel/nf-dbgmodel-imodelmethod-call has a Call method with a _in_opt_ IModelObject *pContextObject parameter.

[damyanp]

Yup - here's a modified version of my example that I think is clearer:

void MyMessageFunc(D3D12_MESSAGE_CATEGORY Category,
    D3D12_MESSAGE_SEVERITY Severity,
    D3D12_MESSAGE_ID ID,
    LPCSTR pDescription,
    _InOut_ void* pContext)
{
    // pContext points to our count of messages
    size_t* messages = reinterpret_cast<size_t*>(pContext);
    *messages = *messages + 1;
}

[riverar]

Ah, if that's also a supported scenario in D3D12, then agreed!

The details are fuzzy re: who's on the hook here since we ingest an external Agility SDK. Maybe we bias to action here and file a change request on Agility SDK repository + fix it locally so folks get immediate benefits. (We'll have to keep an eye out for regression risk on the next sdk ingest.)

[damyanp]

I'm following up with the D3D team now.

[damyanp]

While doing that, I think that getting the correct SAL on this won't necessarily resolve the original problem reported here which is specific to Rust. The SAL captures what may/may not happen in the general case, but Rust has the additional policy that values shared between threads should be const. So, I'm wondering if doing something different for Rust might be appropriate here?

[kennykerr]

Rust turns non-output pointer parameters into *const T otherwise pointer parameters will be *mut T.

In this case, the metadata looks like this:

unsafe HRESULT RegisterMessageCallback([In] D3D12MessageFunc CallbackFunc, [In] D3D12_MESSAGE_CALLBACK_FLAGS CallbackFilterFlags, [In] void* pContext, [In][Out] uint* pCallbackCookie);

Notice the pContext parameter is [In] whereas the pCallbackCookie is [In][Out].

So in Rust it ends up like this:

fn RegisterMessageCallback(&self, callbackfunc: D3D12MessageFunc, callbackfilterflags: D3D12_MESSAGE_CALLBACK_FLAGS, pcontext: *const ::core::ffi::c_void, pcallbackcookie: *mut u32) -> ::windows_core::Result<()>;

Notice the pContext parameter is *const whereas the pCallbackCookie is *mut.

[damyanp]

Right. I'm recommending we fix both cases to explicitly be _InOut_, which means rust will turn them into *mut T.

My concern is that while this is the correct annotation in general for Win32Metadata, a Rust projection may want to consider making them *const T because we happen to know that the callback will likely be called from multiple threads simultaneously. This information isn't captured in the Win32Metadata.

[kennykerr]

For that I think we'd need metadata that says this parameter is shared as opposed to merely transferred or assume that all pointers are shared. But I'm not sure the aliasing rules apply to raw pointers so this may be a non-issue.

[kennykerr]

Specifically, the optimizations and assumptions around aliasing are for shared & and exclusive &mut references rather than pointers.

[damyanp]

The next Agility SDK will have RegisterMessageCallback updated to match the callback:

    HRESULT RegisterMessageCallback(
        [annotation("_In_")]    D3D12MessageFunc CallbackFunc, 
        [annotation("_In_")]    D3D12_MESSAGE_CALLBACK_FLAGS CallbackFilterFlags,
        [annotation("_Inout_")] void* pContext, 
        [annotation("_Inout_")] DWORD *pCallbackCookie);

[mikebattista]

It doesn't look like #1727 included this change. Were you expecting this to be in there?

[alexgong14]

It doesn't look like #1727 included this change. Were you expecting this to be in there?

Thanks for pointing this out. This change was intended to be there, but I missed it. It will be included in the next update, 1.611.1. (I expect there to be a 1.611.1 update, but if there's not, it will be in 1.612)