Skip to content

Detect SPIR-V endianness from the magic number - #1

Closed
afxgroup wants to merge 6 commits into
derfsss:mainfrom
afxgroup:main
Closed

afxgroup wants to merge 6 commits into
derfsss:mainfrom
afxgroup:main

Conversation

@afxgroup

Copy link
Copy Markdown
Contributor

Detect SPIR-V endianness from the magic number and swap only when needed. Per the Vulkan spec, the magic word may be supplied in either endianness — the driver must accept both. The previous unconditional byte-swap was wrong for callers that already produce host-byte-order SPIR-V (e.g. uint32_t array literals compiled on a big-endian host like ImGui's _glsl_shader*_spv on PowerPC). Reading the first word as raw bytes gives us a byte-order-independent way to decide.

Added a software_icd makefile when not using docker

@derfsss

derfsss commented May 11, 2026

Copy link
Copy Markdown
Owner

Thanks Andrea. After review I've landed two of the three commits on a new develop branch and held the third pending a tweak. Status:

✅ Accepted onto develop

  • 46f5baa (your b2a94e7) — Detect SPIR-V endianness from the magic number. Clean win; brings the SW ICD in line with the existing GPU ICD check.
  • 8fd7697 (your 22fc702) — ogles2_vk: SPIR-V endianness in GPU ICD, varying linkage (FORCE_FLATTENED_IO_BLOCKS + interface-block rename to vary), VkFormat 37 / R8G8B8A8_UNORM, VMA allocation/alignment limits, D(...) debug-print plumbing. All correct fixes.
    • Conflict on ogles2_icd/Makefile.cross resolved by dropping the file (it was created in the held commit and modified here; couldn't apply standalone — see follow-up below).

⏸ Held — 3a0f792 (proc-addr fallback removal)

The diagnosis is right: returning APICALL trampolines from vkGetDeviceProcAddr corrupts arguments at the call site (Self in r3 shifts everything by one register). But replacing the fallback with return NULL regresses WSI:

vkCreateSwapchainKHR, vkDestroySwapchainKHR, vkGetSwapchainImagesKHR, vkAcquireNextImageKHR, and vkQueuePresentKHR are present in ogles2vk_LookupProcAddr's DISPATCH(...) table (lines 1565–1569) but absent from ogles2vk_LookupRawProcAddr's RAW(...) table. With the fallback gone, any caller resolving WSI by raw PFN_vk* pointer gets NULL → swapchain creation fails (ImGui's Vulkan backend, vulkaninfo, anything portable).

The fix is to expand the RAW table to mirror DISPATCH (point each entry at the non-_t_ underlying function — all of them already exist) and then drop the fallback. Roughly ~170 mechanical lines. The SW ICD's swvk_LookupRawProcAddr is missing the same WSI funcs and should get the same treatment in the same pass.

Happy to take a revised commit from you, or I'll do it and credit you. No rush.

🛠 Not taken

  • The %d → %ld / (int) → (long) DebugPrintF edits in the loader. IExec->DebugPrintF on OS4 is the C-style printf-class function (the codebase already uses %d/%u widely without trouble), and on PPC32 AmigaOS int == long == 32-bit anyway — the diff is cosmetic with no behaviour change.
  • ogles2_icd/Makefile.cross — as above, couldn't apply standalone; also needs a tidy before landing: the header comment is copy-pasted from software_icd, SOURCES/SDK/DOCKER_* are declared but unused, and the SPIRV-Cross sed -i patch belongs in a one-shot prep step rather than the build recipe. Happy to take a cleaned-up version.

Follow-up 5f33988 already on develop: PR #1 commit 3 added DEBUG := to the three Docker Makefiles but they hardcode their gcc invocations rather than expanding $(CFLAGS), so the new variable was inert in the canonical build. Spliced $(DEBUG) into the recipes so make DEBUG=-DDEBUG activates the D(...) traces. Also reverted software_icd/Makefile.cross's clean: rm -f build/* to the narrower rm -f build/swvk_*.o build/software_vk.library to avoid wiping co-tenant artifacts.

Closing this PR — the curated set lives on develop (https://github.com/derfsss/VulkanOS4/tree/develop). Once the proc-addr revision lands there, I'll fast-forward main.

@derfsss derfsss closed this May 11, 2026
derfsss added a commit that referenced this pull request May 11, 2026
The ICD's vkGetDeviceProcAddr must return raw (non-APICALL) C-ABI
function pointers — the application calls them directly as PFN_vkXxx
with no Self in r3. ogles2vk_LookupRawProcAddr previously listed only
~25 functions and fell back to ogles2vk_LookupProcAddr, which returns
APICALL trampolines. Those trampolines expect Self in r3, so calling
them as raw PFN_vkXxx slides every argument by one register slot —
manifesting as NULL pCreateInfo/pSwapchain inside the ICD when an
application resolved e.g. vkCreateSwapchainKHR through vkGetDeviceProcAddr.

This is the bug PR #1 commit 2 ("Do NOT fall back to ogles2vk_LookupProcAddr")
correctly diagnosed but incorrectly fixed: replacing the fallback with
NULL regressed the five WSI swapchain entry points (and ~165 others)
that lived only in the DISPATCH table.

Expand the ogles2_icd RAW table to mirror the full DISPATCH table —
each entry now points at the underlying non-_t_ function. Then drop
the trampoline fallback in favour of returning NULL.

Also extend software_icd's RAW table to cover the four WSI surface
queries and five swapchain entry points that were similarly missing.

Co-authored-by: Andrea Palmate' <andrea.palmate@gmail.com>
derfsss added a commit that referenced this pull request May 11, 2026
Add inline source-code references to #1 and afxgroup
(Andrea Palmate') in the comments above the expanded RAW tables and
the new WSI entries — he diagnosed the underlying ABI bug.
derfsss added a commit that referenced this pull request May 11, 2026
vulkan.library, software_vk.library, ogles2_vk.library all advance
from 1.2 (21.03.2026) to 1.3 (11.05.2026). The release groups together
the curated parts of afxgroup's PR #1 (endianness, varying linkage,
VMA limits, VkFormat 37, D() macros) with the proc-addr ABI fix that
expands the RAW lookup tables to mirror DISPATCH.

Verified: ELF MSB headers intact on all three, version strings land
correctly in the built binaries.
@afxgroup

Copy link
Copy Markdown
Contributor Author

The diagnosis is right: returning APICALL trampolines from vkGetDeviceProcAddr corrupts arguments at the call site (Self in r3 shifts everything by one register). But replacing the fallback with return NULL regresses WSI:

vkCreateSwapchainKHR, vkDestroySwapchainKHR, vkGetSwapchainImagesKHR, vkAcquireNextImageKHR, and vkQueuePresentKHR are present in ogles2vk_LookupProcAddr's DISPATCH(...) table (lines 1565–1569) but absent from ogles2vk_LookupRawProcAddr's RAW(...) table. With the fallback gone, any caller resolving WSI by raw PFN_vk* pointer gets NULL → swapchain creation fails (ImGui's Vulkan backend, vulkaninfo, anything portable).

The fix is to expand the RAW table to mirror DISPATCH (point each entry at the non-_t_ underlying function — all of them already exist) and then drop the fallback. Roughly ~170 mechanical lines. The SW ICD's swvk_LookupRawProcAddr is missing the same WSI funcs and should get the same treatment in the same pass.

Unfortunately this will break the code. vkCreateSwapchainKHR is an example where everything was started. In a piece of code I have I was getting an error because swap chain was not created correctly because the parameters passed to the underlying functions where NULL except for the first one (that most probably is wrong because it should be the interface pointer). And you have the same behavior in the software rendering where NULL is returned instead of using trampoline functions.
And in fac the game with software icd was working while using ogles2 icd was causing the error.

Regeard debug. I've forgot to add it to CFLAGS too and swap the order. I will fix it

@derfsss

derfsss commented May 13, 2026

Copy link
Copy Markdown
Owner

Heads up — the curated set from this PR shipped as part of v1.3 (main at 64715d7).

Specifically merged from your work, with Co-authored-by: attribution on each:

  • 46f5baa — SPIR-V endianness detection in the SW ICD (your b2a94e7)
  • 8fd7697 — GPU ICD endianness + FORCE_FLATTENED_IO_BLOCKS varying-linkage fix + VK_FORMAT_R8G8B8A8_UNORM + VMA allocation/alignment limits + D(...) debug-print plumbing (your 22fc702)
  • 980d7ad — the vkGetDeviceProcAddr ABI fix, but with the RAW table expanded to mirror DISPATCH before dropping the trampoline fallback (so the five WSI swapchain entry points and ~165 other DISPATCH-only functions don't return NULL). Your diagnosis was correct; the fix is what we discussed above.

Followed up with FIFO vsync handling (f92c842), real glFinish CPU/GPU sync in vkWaitForFences (db065f8), and a -d N duration flag on all examples for automated test sweeps.

Specifically for the ABI bug: there's now a 24_proc_addr example dedicated to this case — it resolves device functions via vkGetDeviceProcAddr and calls each as a raw PFN_vk* across seven categories (control, queue, memory, command, sync, WSI, negative). 23 assertions, including comparing raw-call queue handles against inline-macro queue handles byte-for-byte. So if the DISPATCH/RAW tables ever drift out of sync again, this example fails on the first regression run. PASS 23/23 verified on both qemu-sam460 SW ICD and real X5000 GPU ICD.

Thanks again for the diagnosis and the persistence on the v2 review — your hands-on observations were what made the difference.

derfsss added a commit that referenced this pull request Jun 6, 2026
Integrate the afxgroup collaborator PRs (#1-3) into main alongside the existing
software_icd fixes:
- SPIR-V endianness auto-detect from the magic word (accept BE or LE streams).
- ogles2_vk: do not fall back to ogles2vk_LookupProcAddr() (returns APICALL
  trampolines, not raw PFN_vkXxx) - return NULL for unknown names instead.
- ogles2_vk: proc-addr ABI, varying linkage, VkFormat 37, VMA-limits fixes.

Conflicts resolved keeping BOTH sides: the software_vk shader-module ref-count
(UAF) fix + the Vulkan Y-down viewport convention, AND the ogles2_vk vsync/
glFinish (FIFO present pacing) work, AND the afxgroup changes. Merged software
ICD builds clean; ogles2vk_exec.c syntax-checks (ogles2vk_main.c needs the
Warp3DNova GPU SDK header, absent from the CI image).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants