From patch 1/5:
@@ -1926,6 +1926,12 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(const struct vkd3d_shader_instructio shader_get_type_prefix(shader_version->type), shader_version->major, shader_version->minor, compiler.colours.reset); + if (shader_version->major >= 6) + { + FIXME("Shader model 6 tracing is not implemented yet.\n"); + return VKD3D_ERROR_NOT_IMPLEMENTED; + } + indent = 0; for (i = 0; i < instructions->count; ++i) {
Do we (still) need this? I think tracing DXIL should largely work now.
+ if (byte_code[4] < 16 || byte_code[4] >= byte_code_size) + { + WARN("Invalid bitcode chunk offset %#x (data size %zu).\n", byte_code[4], byte_code_size); + vkd3d_shader_error(message_context, &location, VKD3D_SHADER_ERROR_DXIL_INVALID_CHUNK_OFFSET, + "DXIL bitcode chunk has invalid offset %#x (data size %#zx).", byte_code[4], byte_code_size); + return VKD3D_ERROR_INVALID_SHADER; + } + if (byte_code[4] + byte_code[5] > byte_code_size) + { + WARN("Invalid bitcode chunk size %#x (data size %zu, chunk offset %#x).\n", + byte_code[5], byte_code_size, byte_code[4]); + vkd3d_shader_error(message_context, &location, VKD3D_SHADER_ERROR_DXIL_INVALID_CHUNK_SIZE, + "DXIL bitcode chunk has invalid size %#x (data size %#zx, chunk offset %#x).", + byte_code[5], byte_code_size, byte_code[4]); + return VKD3D_ERROR_INVALID_SHADER; + }
"byte_code[4] + byte_code[5]" can potentially overflow. Since we've already verified that "byte_code[4]" is smaller than "byte_code_size", we can write the condition above as "if (byte_code[5] > byte_code_size - byte_code[4])" instead. Even better would be to introduce actual variable names instead of using magic array offsets.
+ sm6->start = (const uint32_t *)((const char*)&byte_code[2] + byte_code[4]); + if (sm6->start[0] != BITCODE_MAGIC) + WARN("Unrecognised magic number 0x%08x.\n", sm6->start[0]); + + sm6->end = &sm6->start[(byte_code[5] + sizeof(*sm6->start) - 1) / sizeof(*sm6->start)]; + + if ((version.type = version_token >> 16) >= VKD3D_SHADER_TYPE_COUNT) + FIXME("Unrecognised shader type %#x.\n", version.type);
I imagine we might as well use vkd3d_shader_error() for these as well.
+ if ((ret = dxil_block_init(block, NULL, sm6)) < 0) + { + if (ret == VKD3D_ERROR_OUT_OF_MEMORY) + vkd3d_shader_error(message_context, &location, VKD3D_SHADER_ERROR_DXIL_OUT_OF_MEMORY, + "Out of memory parsing DXIL bitcode chunk."); + else if (ret == VKD3D_ERROR_INVALID_SHADER) + vkd3d_shader_error(message_context, &location, VKD3D_SHADER_ERROR_DXIL_INVALID_BITCODE, + "DXIL bitcode chunk has invalid bitcode."); + else + vkd3d_unreachable(); + return ret; + }
Is there any reason we couldn't generate the appropriate errors at the point they occur? (E.g., in dxil_block_read().)
@@ -1088,6 +1090,12 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info vkd3d_shader_trace(&parser->instructions, &parser->shader_version); } + if (parser->shader_version.major >= 6) + { + FIXME("DXIL scanning is not implemented yet.\n"); + return VKD3D_ERROR_NOT_IMPLEMENTED; + } + for (i = 0; i < parser->instructions.count; ++i) { instruction = &parser->instructions.elements[i];
Like further above, do we still need to explicitly handle that here?
@@ -1109,7 +1117,10 @@ static int scan_dxbc(const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_parser *parser; int ret; - if ((ret = vkd3d_shader_sm4_parser_create(compile_info, message_context, &parser)) < 0) + ret = (compile_info->source_type == VKD3D_SHADER_SOURCE_DXBC_DXIL) + ? vkd3d_shader_sm6_parser_create(compile_info, message_context, &parser) + : vkd3d_shader_sm4_parser_create(compile_info, message_context, &parser); + if (ret < 0) { WARN("Failed to initialise shader parser.\n"); return ret; @@ -1156,6 +1167,7 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char switch (compile_info->source_type) { + case VKD3D_SHADER_SOURCE_DXBC_DXIL: case VKD3D_SHADER_SOURCE_DXBC_TPF: ret = scan_dxbc(compile_info, &message_context); break;
I don't really see the point of handling DXIL in scan_dxbc(); the DXIL and TPF paths are about as different as the TPF and D3DBC paths. That largely applies to the change to compile_dxbc_tpf() as well, with the added point that that change makes to function misnamed.
From patch 2/5:
+ /** + * Autodetect VKD3D_SHADER_SOURCE_DXBC_TPF or VKD3D_SHADER_SOURCE_DXBC_DXIL. + * \since 1.9 + */ + VKD3D_SHADER_SOURCE_DXBC_AUTODETECT,
Do we need this? And perhaps more importantly, if we do need this, do we need this right away? For the purpose of vkd3d-compiler and vkd3d, we can just use vkd3d_shader_parse_dxbc() now, without making this part of the public API.
From patch 3/5:
+ {VKD3D_SHADER_SOURCE_DXBC_AUTODETECT, + "dxbc-auto", "Autodetect a 'TPF' or 'DXIL' shader embedded in a DXBC container.\n", + true, VKD3D_SHADER_TARGET_SPIRV_BINARY},
Along a similar vein, does this add much over the existing autodetection we have in vkd3d-compiler?