Add missing _COM_Outptr_ SAL to DxcCreateInstance and friends

[MarijnS95]

Ever since windows-rs started requiring SAL for properly processing functions that return a COM instance these annotations had to be added. They seem to be present on most functions and were worked around for DxcCreateInstance(2) in win32metadata (yet their *Proc typedef variants were omitted).

This includes more random functions that were spotted (particularly the _Out_ IDxc* pattern) but I have been unable to check the nullability for every variant. As per the documentation these _COM_Outptr_ annotations already include the notion of leaving/setting this output pointer to NULL in an error case, in which case a non-S_OK HRESULT should also be returned.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.1797 completed (commit 278ca36d3e by @MarijnS95)

[damyanp]

This PR was closed as it has not been updated in the last two years. Please feel free to reopen if this PR should be merged and is in a reviewable state.

[MarijnS95]

@damyanp A contributor cannot reopen a PR after it has been closed by a collaborator or member. If I were to rebase and (force)push to the branch GitHub dissociates it from the PR, never allowing it to be reopened again.

Would you be able to help out reviewing and getting this PR merged if I rebase it? Is this repository even the right place (source of truth) for most of these headers before they ultimately end up in the Windows SDK (which is the ultimate goal to get picked up by win32metadata and windows-rs)?

[damyanp]

I'll help shepherd this one through. This repo is the source of truth for dxcapi.h and DxbcConverter.h, so if we get it merged they'll eventually flow through to where win32metadata / windows-rs will see it.

I think you're saying that you'll need to create a new PR if you rebase this? That should be fine.

[MarijnS95]

I'll help shepherd this one through. This repo is the source of truth for dxcapi.h and DxbcConverter.h, so if we get it merged they'll eventually flow through to where win32metadata / windows-rs will see it.

Awesome, thanks!

I think you're saying that you'll need to create a new PR if you rebase this? That should be fine.

I would be able to reuse this PR and "context" if you reopen it before I rebase and force-push :)

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff e7346acd9d4000035d8383c713ee966a5abdd7cb 68eb68a70a9312a8813feda36a525efe84d2f217 -- include/dxc/DxilContainer/DxcContainerBuilder.h include/dxc/dxcapi.h include/dxc/dxcapi.internal.h include/dxc/dxcdxrfallbackcompiler.h include/dxc/dxcisense.h projects/dxilconv/include/DxbcConverter.h projects/dxilconv/lib/DxbcConverter/DxbcConverter.cpp projects/dxilconv/lib/DxbcConverter/DxbcConverterImpl.h tools/clang/tools/dxcompiler/dxcapi.cpp tools/clang/tools/dxrfallbackcompiler/dxcapi.cpp

View the diff from clang-format here.

diff --git a/include/dxc/dxcapi.h b/include/dxc/dxcapi.h
index 341ef29b..11e7b49c 100644
--- a/include/dxc/dxcapi.h
+++ b/include/dxc/dxcapi.h
@@ -71,17 +71,18 @@ typedef HRESULT(__stdcall *DxcCreateInstance2Proc)(_In_ IMalloc *pMalloc,
 ///
 /// While this function is similar to CoCreateInstance, there is no COM
 /// involvement.
-extern "C" DXC_API_IMPORT HRESULT __stdcall
-DxcCreateInstance(_In_ REFCLSID rclsid, _In_ REFIID riid,
-                  _COM_Outptr_ LPVOID *ppv);
+extern "C" DXC_API_IMPORT
+    HRESULT __stdcall DxcCreateInstance(_In_ REFCLSID rclsid, _In_ REFIID riid,
+                                        _COM_Outptr_ LPVOID *ppv);
 
 /// \brief Version of DxcCreateInstance that takes an IMalloc interface.
 ///
 /// This can be used to create an instance of the compiler with a custom memory
 /// allocator.
-extern "C" DXC_API_IMPORT HRESULT __stdcall
-DxcCreateInstance2(_In_ IMalloc *pMalloc, _In_ REFCLSID rclsid,
-                   _In_ REFIID riid, _COM_Outptr_ LPVOID *ppv);
+extern "C" DXC_API_IMPORT
+    HRESULT __stdcall DxcCreateInstance2(_In_ IMalloc *pMalloc,
+                                         _In_ REFCLSID rclsid, _In_ REFIID riid,
+                                         _COM_Outptr_ LPVOID *ppv);
 
 // For convenience, equivalent definitions to CP_UTF8 and CP_UTF16.
 #define DXC_CP_UTF8 65001
diff --git a/projects/dxilconv/include/DxbcConverter.h b/projects/dxilconv/include/DxbcConverter.h
index d1cd8dcf..50ec7612 100644
--- a/projects/dxilconv/include/DxbcConverter.h
+++ b/projects/dxilconv/include/DxbcConverter.h
@@ -17,16 +17,15 @@
 #ifndef _MSC_VER
 extern "C"
 #endif
-    DXC_API_IMPORT HRESULT __stdcall
-    DxcCreateInstance(_In_ REFCLSID rclsid, _In_ REFIID riid,
-                      _COM_Outptr_ LPVOID *ppv);
+    DXC_API_IMPORT HRESULT __stdcall DxcCreateInstance(
+        _In_ REFCLSID rclsid, _In_ REFIID riid, _COM_Outptr_ LPVOID *ppv);
 
 #ifndef _MSC_VER
 extern "C"
 #endif
-    DXC_API_IMPORT HRESULT __stdcall
-    DxcCreateInstance2(_In_ IMalloc *pMalloc, _In_ REFCLSID rclsid,
-                       _In_ REFIID riid, _COM_Outptr_ LPVOID *ppv);
+    DXC_API_IMPORT HRESULT __stdcall DxcCreateInstance2(
+        _In_ IMalloc *pMalloc, _In_ REFCLSID rclsid, _In_ REFIID riid,
+        _COM_Outptr_ LPVOID *ppv);
 
 struct __declspec(uuid("5F956ED5-78D1-4B15-8247-F7187614A041")) IDxbcConverter
     : public IUnknown {
diff --git a/tools/clang/tools/dxrfallbackcompiler/dxcapi.cpp b/tools/clang/tools/dxrfallbackcompiler/dxcapi.cpp
index 10b6a74e..3c62556b 100644
--- a/tools/clang/tools/dxrfallbackcompiler/dxcapi.cpp
+++ b/tools/clang/tools/dxrfallbackcompiler/dxcapi.cpp
@@ -34,9 +34,9 @@ static HRESULT ThreadMallocDxcCreateInstance(REFCLSID rclsid, REFIID riid,
   return hr;
 }
 
-DXC_API_IMPORT HRESULT __stdcall
-DxcCreateDxrFallbackCompiler(REFCLSID rclsid, REFIID riid,
-                             _COM_Outptr_ LPVOID *ppv) {
+DXC_API_IMPORT
+    HRESULT __stdcall DxcCreateDxrFallbackCompiler(REFCLSID rclsid, REFIID riid,
+                                                   _COM_Outptr_ LPVOID *ppv) {
   if (ppv == nullptr) {
     return E_POINTER;
   }

• Check this box to apply formatting changes to this branch.

[MarijnS95]

I've very quickly gone through a first rebase to clean up conflicts. Lots of existing SAL seems to have been removed (either because it was redundant or not relevant in internal files). TODO:

• Need to figure out if some files should be excluded, i.e. .cpp files and "internal" headers (though those existing _Out_ ones might still be relevant to replace);
• Difference between _COM_Outptr_ vs _Outptr_result_nullonfailure_;
• Check internal implementations when _opt and _result_maybenull should be used.

See also https://learn.microsoft.com/en-us/cpp/code-quality/annotating-function-parameters-and-return-values.

[damyanp]

Looks like some formatting stuff needed. In the meantime, I was wondering what you'd think about limiting this change to just the header files that are included in the SDK, just to be super cautious of churn? I think that would just be dxcapi.h?

[damyanp]

Sorry, I didn't see your last comment when I posted my previous message where you'd already called out the point I was raising!

In places where the SAL has been removed we shouldn't add it back. It should have been removed from all internal uses - only the external headers that get included in releases / the SDK should be annotated.

[MarijnS95]

Cool, so tools/clang/tools/dxcompiler/dxcapi.cpp shouldn't be changed.

tools/clang/tools/dxcompiler/dxcapi.cpp however has what I believe to be wrong _Out_ annotations that should be _COM_Outptr_.

projects/dxilconv/lib/DxbcConverter/DxbcConverterImpl.h as a header misses all annotations too (but I might not need to repeat declaration annotations at definition time in projects/dxilconv/lib/DxbcConverter/DxbcConverter.cpp).

Everything in the root include/ folder should be annotated however, as I assume those become the public SDK.

Finally, include/dxc/dxcisense.h seems to have a bunch of "mistakes" that need to be clarified.

---

Regarding formatting, my local clang-format seems to be a different/newer version; will patch this up by hand.

[damyanp]

Generally, we're trying to avoid SAL annotations as much as possible - so we're only including them where there are clear needs for tooling to use them.

My preference here would be for a minimal change - just enough so that the workarounds in win32metadata can be removed. So this would just involve updating DxcCreateInstance and DxcCreateInstance2.

If there are reasons other than aiming for completeness that are motivating updating more than this then please let me know :).

Cool, so tools/clang/tools/dxcompiler/dxcapi.cpp shouldn't be changed.

If there are implementations of functions defined in the header then we should make sure the SAL matches - so if we're changing dxcapi.h we may need to change the cpp as well.

projects/dxilconv/lib/DxbcConverter/DxbcConverterImpl.h as a header misses all annotations too (but I might not need to repeat declaration annotations at definition time in projects/dxilconv/lib/DxbcConverter/DxbcConverter.cpp).

We shouldn't modify this file.

Everything in the root include/ folder should be annotated however, as I assume those become the public SDK.

You'd think so, but it turns out that there are many files in that include folder that aren't published. The only file that goes into the SDK is dxcapi.h.

Some additional files are included in the nuget / zip release packages:

• dxcerrors.h
• dxcisense.h

But I'm not sure these are worth doing anything with since win32metadata isn't touching them (and long term future of dxcisense.h is TBD).

Regarding formatting, my local clang-format seems to be a different/newer version; will patch this up by hand.

There's a check box in the comment that reported the error, if you click that then it'll just fix it for you. The confusing thing is that the same comment / check box is reused every time you push new changes, so you need to scroll back up to that comment if you need to have it fixed again.