Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIR-V] Access violation error while parsing the shader #6689

Closed
TheArheus opened this issue Jun 11, 2024 · 5 comments · Fixed by #6698
Closed

[SPIR-V] Access violation error while parsing the shader #6689

TheArheus opened this issue Jun 11, 2024 · 5 comments · Fixed by #6698
Assignees
Labels
bug Bug, regression, crash spirv Work related to SPIR-V

Comments

@TheArheus
Copy link

TheArheus commented Jun 11, 2024

I've got an issue while doing my shader. The shader had Texture2DArray and TextureCubeArray combined with samplers. The issue is that I've got this error:
Exception thrown: read access violation.
this was nullptr.
On this line:

>	dxcompiler.dll!spvtools::opt::Instruction::opcode() Line 238	C++
 	dxcompiler.dll!spvtools::opt::Instruction::GetBaseAddress() Line 246	C++
 	dxcompiler.dll!spvtools::opt::MemPass::GetPtr(unsigned int ptrId, unsigned int * varId) Line 108	C++
 	dxcompiler.dll!spvtools::opt::MemPass::GetPtr(spvtools::opt::Instruction * ip, unsigned int * varId) Line 134	C++
 	dxcompiler.dll!spvtools::opt::LocalSingleBlockLoadStoreElimPass::LocalSingleBlockLoadStoreElim(spvtools::opt::Function * func) Line 71	C++
 	dxcompiler.dll!spvtools::opt::LocalSingleBlockLoadStoreElimPass::ProcessImpl::__l2::<lambda>(spvtools::opt::Function * fp) Line 220	C++
 	dxcompiler.dll!std::invoke<bool <lambda>(spvtools::opt::Function *) &,spvtools::opt::Function *>(spvtools::opt::LocalSingleBlockLoadStoreElimPass::ProcessImpl::__l2::bool <lambda>(spvtools::opt::Function *) & _Obj, spvtools::opt::Function * && _Arg1) Line 1756	C++
 	dxcompiler.dll!std::_Func_impl_no_alloc<bool <lambda>(spvtools::opt::Function *),bool,spvtools::opt::Function *>::_Do_call(spvtools::opt::Function * && <_Args_0>) Line 812	C++
 	dxcompiler.dll!std::_Func_class<bool,spvtools::opt::Function *>::operator()(spvtools::opt::Function * <_Args_0>) Line 855	C++
 	dxcompiler.dll!spvtools::opt::IRContext::ProcessCallTreeFromRoots(std::function<bool __cdecl(spvtools::opt::Function *)> & pfn, std::queue<unsigned int,std::deque<unsigned int,std::allocator<unsigned int>>> * roots) Line 985	C++
 	dxcompiler.dll!spvtools::opt::IRContext::ProcessReachableCallTree(std::function<bool __cdecl(spvtools::opt::Function *)> & pfn) Line 970	C++
 	dxcompiler.dll!spvtools::opt::LocalSingleBlockLoadStoreElimPass::ProcessImpl() Line 222	C++
 	dxcompiler.dll!spvtools::opt::LocalSingleBlockLoadStoreElimPass::Process() Line 232	C++
 	dxcompiler.dll!spvtools::opt::Pass::Run(spvtools::opt::IRContext * ctx) Line 37	C++
 	dxcompiler.dll!spvtools::opt::PassManager::Run(spvtools::opt::IRContext * context) Line 58	C++
 	dxcompiler.dll!spvtools::Optimizer::Run(const unsigned int * original_binary, const unsigned __int64 original_binary_size, std::vector<unsigned int,std::allocator<unsigned int>> * optimized_binary, spv_optimizer_options_t * const opt_options) Line 684	C++
 	dxcompiler.dll!clang::spirv::SpirvEmitter::spirvToolsLegalize(std::vector<unsigned int,std::allocator<unsigned int>> * mod, std::string * messages, const std::vector<spvtools::opt::DescriptorSetAndBinding,std::allocator<spvtools::opt::DescriptorSetAndBinding>> * dsetbindingsToCombineImageSampler) Line 14603	C++
 	dxcompiler.dll!clang::spirv::SpirvEmitter::HandleTranslationUnit(clang::ASTContext & context) Line 907	C++
 	dxcompiler.dll!clang::ParseAST(clang::Sema & S, bool PrintStats, bool SkipFunctionBodies) Line 166	C++
 	dxcompiler.dll!clang::ASTFrontendAction::ExecuteAction() Line 556	C++
 	dxcompiler.dll!clang::FrontendAction::Execute() Line 468	C++
 	dxcompiler.dll!DxcCompiler::Compile(const DxcBuffer * pSource, const wchar_t * * pArguments, unsigned int argCount, IDxcIncludeHandler * pIncludeHandler, const _GUID & riid, void * * ppResult) Line 974	C++

My shader for this case, I've tried to reproduce the issue with:

[[vk::combinedImageSampler]]
Texture2DArray<float4> GBuffer : register(t4, space0);
[[vk::combinedImageSampler]]
SamplerState _GBuffer_sampler : register(s4, space0);
RWTexture2D<float4> OcclusionTarget : register(u5, space0);


[numthreads(32, 32, 1)]
void main(uint3 GlobalInvocationID : SV_DispatchThreadID)
{
    float2 TextureDims;
    GBuffer.GetDimensions(0, TextureDims.x, TextureDims.y, 0, 0);
}

One thing is that shader cannot be compiled with both usage of Texture2DArray functions. But, if I remove one of those, it will.

I am compiling this shader with this command:
dxc -spirv -E main -T cs_6_6 ..\shader.comp.hlsl

@TheArheus TheArheus added bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V labels Jun 11, 2024
@TheArheus TheArheus closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
@TheArheus TheArheus reopened this Jun 12, 2024
@devshgraphicsprogramming

AFAIK vk::combinedImageSampler is deprecated but the syntax for declaring them "properly" with Proposal 0011 (inline SPIR-V) segfaults.

Anyway in order for your shader to compile you need to declare the combined sampler twice, and probably use vk::binding() instead of relying on automagic HLSL register to SPIR-V location translation

@TheArheus
Copy link
Author

Hmm, got it. Because I wanted to do something like this:

Texture2D<float4> GBuffer[5] : register(t4, space0);
SamplerState _GBuffer_sampler[5] : register(s4, space0);

But, I saw, that I should use Texture2DArray and one sampler and vk::combinedImageSampler. So, then, if it is deprecated, then, the example in the wiki should not be correct anymore? I am confused about the right way to do it with the modern versions

@devshgraphicsprogramming

I think there's a bug about arrays of combined image samplers #5092

This made us support separate samplers in our engine.

@s-perron
Copy link
Collaborator

I want to deprecate vk::combinedImageSampler, but we don't have the replacement done yet. We will fix small bugs for the feature, but will not implement bigger changes. For example, we will not add the ability to do arrays of combined texture samplers. We'll take a look at this, and see where the problem it. It is getting the access violation in the optimizer, probably because something it returning a nullptr. This is worth looking into.

@s-perron s-perron removed the needs-triage Awaiting triage label Jun 17, 2024
@s-perron s-perron self-assigned this Jun 17, 2024
@s-perron
Copy link
Collaborator

The problem is not related to the texture samplers. The problem is that the HLSL is invalid, and the SPIR-V backend is not catching the problem.

You error is still there even if I remove the combinedtexturesampler attributes. If I compile the shader for DXIL, it gives the proper error:

t.hlsl:10:60: error: cannot compile this unexpected cast lvalue yet
    GBuffer.GetDimensions(0, TextureDims.x, TextureDims.y, 0, 0);
                                                           ^
t.hlsl:10:63: error: cannot compile this unexpected cast lvalue yet
    GBuffer.GetDimensions(0, TextureDims.x, TextureDims.y, 0, 0);

The last two parameters to this function call are out parameter. See the docs. This is essentially telling the compiler that they should write to the memory lcationo of the contant 0, which makes no sense. However the spir-v tries, and generates:

               OpStore %float_0 %47

This is invalid spir-v, and cause the optimizer to fail.

This fix for this will be to try to add a more meaningful error message, but the test given above will never work. You code will have to be changed.

s-perron added a commit to s-perron/DirectXShaderCompiler that referenced this issue Jun 17, 2024
When processing the GetDimension member function for textures, we do not
emit an error if the output variable is not an l-value. This change will
add this error.

Fixes microsoft#6689
s-perron added a commit that referenced this issue Jun 18, 2024
When processing the GetDimension member function for textures, we do not
emit an error if the output variable is not an l-value. This change will
add this error.

Fixes #6689
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash spirv Work related to SPIR-V
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants