Fix D3D12 debug crash due to validation layers SDK bug #3222

RazielXYZ · 2023-12-13T23:15:36Z

This fixes #3176 and the other intermittent questions about this problem.
This way of runtime checking the windows build is the least ugly/hacky that I could find (and yet, it's still awful).
I only tested on win11 build 22621. If we think tests on other versions or builds are necessary I can try to make some VMs.

bkaradzic · 2023-12-14T00:01:00Z

Can you use windowsVersionIs function?

bgfx/src/bgfx.cpp

Lines 2656 to 2682 in e2631c1

    
           	bool windowsVersionIs(Condition::Enum _op, uint32_t _version) 
        
           	{ 
        
           #if BX_PLATFORM_WINDOWS 
        
           		static const uint8_t s_condition[] = 
        
           		{ 
        
           			VER_LESS_EQUAL, 
        
           			VER_GREATER_EQUAL, 
        
           		}; 
        
           		OSVERSIONINFOEXA ovi; 
        
           		bx::memSet(&ovi, 0, sizeof(ovi) ); 
        
           		ovi.dwOSVersionInfoSize = sizeof(ovi); 
        
           		// _WIN32_WINNT_WINBLUE 0x0603 
        
           		// _WIN32_WINNT_WIN8    0x0602 
        
           		// _WIN32_WINNT_WIN7    0x0601 
        
           		// _WIN32_WINNT_VISTA   0x0600 
        
           		ovi.dwMajorVersion = HIBYTE(_version); 
        
           		ovi.dwMinorVersion = LOBYTE(_version); 
        
           		DWORDLONG cond = 0; 
        
           		VER_SET_CONDITION(cond, VER_MAJORVERSION, s_condition[_op]); 
        
           		VER_SET_CONDITION(cond, VER_MINORVERSION, s_condition[_op]); 
        
           		return !!VerifyVersionInfoA(&ovi, VER_MAJORVERSION | VER_MINORVERSION, cond); 
        
           #else 
        
           		BX_UNUSED(_op, _version); 
        
           		return false; 
        
           #endif // BX_PLATFORM_WINDOWS 
        
           	}

RazielXYZ · 2023-12-14T00:13:25Z

Can you use windowsVersionIs function?

Not really because the dwMajorVersion and dwMinorVersion for all builds of windows 10 and 11 are the same (major = 10 and minor = 0) so it doesn't get us the specific enough version/build that we need.
Also, because bgfx examples (and probably lots of user apps) don't use compatibility app manifests, windowsVersionIs always reports major = 6 and minor = 2 (corresponding to windows 8.0) on anything windows 8 or higher.
See: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-verifyversioninfoa "If the application has no manifest, VerifyVersionInfo behaves as if the operation system version is Windows 8 (6.2)."
I would actually suggest maybe using the method in this PR in windowsVersionIs too if we care about the more specific and correct version and build number in other places.

bkaradzic · 2023-12-14T00:27:52Z

src/renderer_d3d12.cpp

+								    if (hMod) {
+										FARPROC (WINAPI* rtlGetVersionPtr) (PRTL_OSVERSIONINFOW) = reinterpret_cast<FARPROC (WINAPI*)(PRTL_OSVERSIONINFOW)>(::GetProcAddress(hMod, "RtlGetVersion"));
+								        if (rtlGetVersionPtr != nullptr) {
+								            rtlGetVersionPtr(&osver);


Before you call rtlGetVersion(&osver) you need to specify dwOSVersionInfoSize size.
https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-rtlgetversion#parameters

bkaradzic · 2023-12-14T00:28:09Z

src/renderer_d3d12.cpp

+									RTL_OSVERSIONINFOW osver;
+									bx::memSet(&osver, 0 , sizeof(RTL_OSVERSIONINFOW));
+									const HMODULE hMod = ::GetModuleHandleW(L"ntdll.dll");
+								    if (hMod) {


Use bgfx code style... braces next line...

bkaradzic · 2023-12-14T00:29:46Z

src/renderer_d3d12.cpp

@@ -844,13 +844,31 @@ namespace bgfx { namespace d3d12

 								if (SUCCEEDED(hr))
 								{
-//									debug1->SetEnableGPUBasedValidation(true);
+									//This is the least ugly way to get the windows build that still works past win10 without deprecation warnings or needing app manifests
+									RTL_OSVERSIONINFOW osver;


Move this into function, make content of function #if BX_PLATFORM_WINDOWS because D3D1x compiles on Linux too.

Move windows version checking to function in bgfx.cpp Change windowsVersionIs to also use this technique Use bgfx code style Specify dwOSVersionInfoSize prior to calling rtlGetVersion

RazielXYZ · 2023-12-14T01:27:48Z

Looks like BX_PLATFORM_WINDOWS and BX_PLATFORM_EMSCRIPTEN are both defined at the same time for emscripten builds - that doesn't seem good (and it breaks the emscripten build due to it trying to compile GetProcAddress)

src/bgfx.cpp

Use bx::dlsym instead of GetProcAddress directly Fix major & minor comparison logic

bkaradzic · 2023-12-14T02:37:45Z

src/bgfx.cpp

+				{
+					return false;
+				}
+			}


Here you could do:

ovi.dwBuildNumber = UINT32_MAX == _build ? UINT32_MAX : ovi.dwBuildNumber;

And then remove those if/else blocks below since test will succeed.

Great idea!

Chris1AA · 2023-12-14T10:13:35Z

Platform, edition and version independency

Creating dependencies to kernel-mode and/or windows-driver functions within an user-app is not a good idea per se, even if Windows calls a particular function indirectly from user-mode.

WDM (windows-driver): https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-rtlgetversion

Win32 (user-mode): https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getversionexa

There always may be runtimes or editions restricting kernel-mode or driver-mode access from an user-app.

You are doing better using the user-mode function GetVersion or GetVersionEx, even it may return the targeted version (manifest or compaitbility version - a proper app needs a manifest anyway!) and the compiler eventually tells you using VerifyVersionInfo in preference.

Alternatively, though with bigger overhead, WMI methods could be used.

RazielXYZ · 2023-12-14T16:06:25Z

I agree in theory. However, I don't see us adding manifests to every example (but maybe we could through example-common entry since that instantiates the window/app), nor do I see users/devs adding manifests to their app (at least not from the get go), which means even with proper usage of VerifyVersionInfo we would get wrong results and the crash would be back - and users unfamiliar with the issue or with the need for manifests would be confused.

GetVersion or GetVersionEx are out of the question because they're deprecated: per your link "GetVersionExA may be altered or unavailable for releases after Windows 8.1."; trying to use it issues deprecated warning, which examples treat as error. Even if they weren't, they have the same behavior as VerifyVersionInfo wherein they need manifests to be correct.

Alternatively, windows build string could be found in the registry or in the file info of windows dlls and other files. However, I don't really think those methods are any less prone to random restrictions or changes in the future, nor are they cleaner.

Chris1AA · 2023-12-14T17:05:34Z

You are right, GetVersionEx causes a depreciation compiler message, which takes the same risk failing in future Windows versions than using Rtl* functions.

A suggested way using VerifyVersionInfo, when passing a value of 20000 or greater to _build, to query for Windows 11 and later:

	bool windowsVersionIs(Condition::Enum _op, uint32_t _version, uint32_t _build = 0)
	{
#if BX_PLATFORM_WINDOWS
		static const uint8_t s_condition[] =
		{
			VER_LESS_EQUAL,
			VER_GREATER_EQUAL,
		};

		OSVERSIONINFOEXA ovi;
		bx::memSet(&ovi, 0, sizeof(ovi) );
		ovi.dwOSVersionInfoSize = sizeof(ovi);
		// _WIN32_WINNT_WINBLUE 0x0603
		// _WIN32_WINNT_WIN8    0x0602
		// _WIN32_WINNT_WIN7    0x0601
		// _WIN32_WINNT_VISTA   0x0600
		ovi.dwMajorVersion = HIBYTE(_version);
		ovi.dwMinorVersion = LOBYTE(_version);
		DWORDLONG cond = 0;
		VER_SET_CONDITION(cond, VER_MAJORVERSION, s_condition[_op]);
		VER_SET_CONDITION(cond, VER_MINORVERSION, s_condition[_op]);
		if (_build)
		{
			ovi.dwBuildNumber = _build;
			VER_SET_CONDITION(cond, VER_BUILDNUMBER, s_condition[_op]);
			return !!VerifyVersionInfoA(&ovi, VER_MAJORVERSION | VER_MINORVERSION | VER_BUILDNUMBER, cond);
		}
		return !!VerifyVersionInfoA(&ovi, VER_MAJORVERSION | VER_MINORVERSION, cond);
#else
		BX_UNUSED(_op, _version);
		return false;
#endif // BX_PLATFORM_WINDOWS
	}

RazielXYZ · 2023-12-14T17:15:05Z

A suggested way using VerifyVersionInfo, when passing a value of 20000 or greater to _build, to query for Windows 11 and later:

Am I correct in assuming that doesn't see/report the build number correctly unless a manifest is present? 😅

Chris1AA · 2023-12-14T19:19:58Z

Right, your solution is the 'smallest bitter pill', unless a manifest is used.
Sorry, since we do use manifests, this was not obvious to me before.

https://learn.microsoft.com/en-us/windows/win32/sysinfo/targeting-your-application-at-windows-8-1

Chris1AA · 2023-12-21T21:24:31Z

@RazielXYZ @bkaradzic

° Suggestion - adding a proper manifest.

Add a generic manifest to the toolchains for all Windows executables and revert the VerifyVersionInfoA as shown above, by:

One standalone manifest to all exes and dlls are sufficient.

Add a generic.bgfx.manifest to the scripts folder containing:

<?xml version='1.0' encoding='UTF-8' standalone='yes'?>
<assembly xmlns='urn:schemas-microsoft-com:asm.v1' manifestVersion='1.0'>
  <assemblyIdentity
    version="1.0.0.0"
    processorArchitecture="*"
    name="bgfx-git.bgfx.demo"
  />
  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1"> 
    <application>
      <!-- Windows 10 -->
      <supportedOS Id="{8e0f7a12-bfb3-4fe8-b9a5-48fd50a15a9a}"/>
      <!-- Windows 8.1 -->
      <supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
      <!-- Windows 8 -->
      <supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
      <!-- Windows 7 -->
      <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
      <!-- Windows Vista -->
      <supportedOS Id="{e2011457-1546-43c5-a5fe-008deee3d3f0}"/>
    </application>
  </compatibility>
</assembly>

2a) Add the manifest file to VS-solution files, either embedded or standalone.

2b) Other IDE's or toolchains define a standalone manifest-file, and copy scripts/generic.bgfx.manifest into the exe's or dll's folder.

bkaradzic · 2023-12-21T23:55:29Z

I don't oppose idea of adding manifest... Can you research what's minimal viable manifest? Like remove all windows OS before 10. We don't care about any of those.

Does name="bgfx-git.bgfx.demo" need to be per executable, or every single executable can share this?

2a) Add the manifest file to VS-solution files, either embedded

We want embedded.

Chris1AA · 2023-12-22T01:25:29Z

I don't oppose idea of adding manifest... Can you research what's minimal viable manifest? Like remove all windows OS before 10. We don't care about any of those.

I fully agree on dropping implicit support of older Windows versions than 10, when Win10 gets 10 y/o already.
Minimum definition for Win10+ embedded manifest:

<?xml version='1.0' encoding='UTF-8'?>
<assembly xmlns='urn:schemas-microsoft-com:asm.v1' manifestVersion='1.0'>
  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1"> 
    <application>
      <!-- Windows 10 -->
      <supportedOS Id="{8e0f7a12-bfb3-4fe8-b9a5-48fd50a15a9a}"/>
    </application>
  </compatibility>
</assembly>

Does name="bgfx-git.bgfx.demo" need to be per executable, or every single executable can share this?

That's not a problem for the demo exes, not at all if the manifest is embedded.

I can provide the needed VS-C compiler command line options tomorrow, after double checking on the minimum requirements.

bkaradzic · 2023-12-22T01:54:01Z

I can provide the needed VS-C compiler command line options tomorrow, after double checking on the minimum requirements.

Yeah, if you could add this to GENie script that would be the best.

RazielXYZ · 2023-12-22T02:01:33Z

Disregarding how this is going to work on mingw or such, we'll also have to make it clear to users that dx12 debug is broken unless they add a similar manifest to their own application. Not sure where the best place to do that would be.

Chris1AA · 2023-12-22T09:31:26Z

@RazielXYZ
I am not into mingw, maybe you can find a solution on how to add a manifest for mingw?

Btw, embedding manifests can be done by a resource as well.

@bkaradzic
Thinking: Since Win10+ should be targeted only, and since this is for D3D12 debugging only when a coder should know what he does anyway, the particular version check might be obsolete and could be bypassed via a define eventually?

I am waiting for you coming up with a solution for mingw, before proceeding on it (prolly after Xmas). In any way, we should go with the easiest, but proper, solution for it - I don't want to over-complicate it.

RazielXYZ · 2023-12-22T17:14:05Z

@Chris1AA I am not into mingw either 😅 not sure who would be familiar with it that we could ask

* Fix D3D12 debug crash due to validation layers SDK bug * Change LONG to FARPROC to be technically correct * Make requested changes: Move windows version checking to function in bgfx.cpp Change windowsVersionIs to also use this technique Use bgfx code style Specify dwOSVersionInfoSize prior to calling rtlGetVersion * Merge both functions into one Use bx::dlsym instead of GetProcAddress directly Fix major & minor comparison logic * Fix win10 version comment * Revert rendererCreate check back to Win 0x0602 * Clean up logic as per suggestion

Fix D3D12 debug crash due to validation layers SDK bug

4758bea

RazielXYZ requested a review from bkaradzic as a code owner December 13, 2023 23:15

Change LONG to FARPROC to be technically correct

37cdf63

bkaradzic requested changes Dec 14, 2023

View reviewed changes

Make requested changes:

191f917

Move windows version checking to function in bgfx.cpp Change windowsVersionIs to also use this technique Use bgfx code style Specify dwOSVersionInfoSize prior to calling rtlGetVersion

bkaradzic requested changes Dec 14, 2023

View reviewed changes

src/bgfx.cpp Outdated Show resolved Hide resolved

RazielXYZ added 3 commits December 14, 2023 04:13

Merge both functions into one

e68e561

Use bx::dlsym instead of GetProcAddress directly Fix major & minor comparison logic

Fix win10 version comment

bab78d8

Revert rendererCreate check back to Win 0x0602

824544f

bkaradzic requested changes Dec 14, 2023

View reviewed changes

Clean up logic as per suggestion

18b5942

bkaradzic merged commit f4a9bfc into bkaradzic:master Dec 14, 2023
11 of 13 checks passed

RazielXYZ deleted the dx12_validation_fix branch December 14, 2023 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix D3D12 debug crash due to validation layers SDK bug #3222

Fix D3D12 debug crash due to validation layers SDK bug #3222

RazielXYZ commented Dec 13, 2023

bkaradzic commented Dec 14, 2023 •

edited

Loading

RazielXYZ commented Dec 14, 2023 •

edited

Loading

bkaradzic Dec 14, 2023

bkaradzic Dec 14, 2023

bkaradzic Dec 14, 2023

RazielXYZ commented Dec 14, 2023

bkaradzic Dec 14, 2023

RazielXYZ Dec 14, 2023

Chris1AA commented Dec 14, 2023

RazielXYZ commented Dec 14, 2023 •

edited

Loading

Chris1AA commented Dec 14, 2023

RazielXYZ commented Dec 14, 2023

Chris1AA commented Dec 14, 2023

Chris1AA commented Dec 21, 2023 •

edited

Loading

bkaradzic commented Dec 21, 2023 •

edited

Loading

Chris1AA commented Dec 22, 2023 •

edited

Loading

bkaradzic commented Dec 22, 2023

RazielXYZ commented Dec 22, 2023

Chris1AA commented Dec 22, 2023 •

edited

Loading

RazielXYZ commented Dec 22, 2023

Fix D3D12 debug crash due to validation layers SDK bug #3222

Fix D3D12 debug crash due to validation layers SDK bug #3222

Conversation

RazielXYZ commented Dec 13, 2023

bkaradzic commented Dec 14, 2023 • edited Loading

RazielXYZ commented Dec 14, 2023 • edited Loading

bkaradzic Dec 14, 2023

Choose a reason for hiding this comment

bkaradzic Dec 14, 2023

Choose a reason for hiding this comment

bkaradzic Dec 14, 2023

Choose a reason for hiding this comment

RazielXYZ commented Dec 14, 2023

bkaradzic Dec 14, 2023

Choose a reason for hiding this comment

RazielXYZ Dec 14, 2023

Choose a reason for hiding this comment

Chris1AA commented Dec 14, 2023

Platform, edition and version independency

RazielXYZ commented Dec 14, 2023 • edited Loading

Chris1AA commented Dec 14, 2023

RazielXYZ commented Dec 14, 2023

Chris1AA commented Dec 14, 2023

Chris1AA commented Dec 21, 2023 • edited Loading

bkaradzic commented Dec 21, 2023 • edited Loading

Chris1AA commented Dec 22, 2023 • edited Loading

bkaradzic commented Dec 22, 2023

RazielXYZ commented Dec 22, 2023

Chris1AA commented Dec 22, 2023 • edited Loading

RazielXYZ commented Dec 22, 2023

bkaradzic commented Dec 14, 2023 •

edited

Loading

RazielXYZ commented Dec 14, 2023 •

edited

Loading

RazielXYZ commented Dec 14, 2023 •

edited

Loading

Chris1AA commented Dec 21, 2023 •

edited

Loading

bkaradzic commented Dec 21, 2023 •

edited

Loading

Chris1AA commented Dec 22, 2023 •

edited

Loading

Chris1AA commented Dec 22, 2023 •

edited

Loading