vkd3d-shader/dxil: Read and validate DXIL bitcode blocks.
Block records are not processed; only the bitcode is validated.
Merge request reports
Activity
- Resolved by Giovanni Mascellani
Marvin replied on the mailing list:
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125596 Your paranoid android. === debian11 (build log) === Task: Patch failed to apply === debian11b (build log) === Task: Patch failed to apply
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
From patch 1/2:
@@ -243,6 +244,7 @@ libvkd3d_shader_la_SOURCES = \ libs/vkd3d-shader/hlsl_sm4.c \ libs/vkd3d-shader/preproc.h \ libs/vkd3d-shader/sm4.h \ + libs/vkd3d-shader/sm6.h \ libs/vkd3d-shader/spirv.c \ libs/vkd3d-shader/trace.c \ libs/vkd3d-shader/vkd3d_shader.map \
sm6.h is only used by dxil.c. Perhaps that will change in future commits, but until it does, its contents simply belong in dxil.c.
@@ -2074,6 +2073,11 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi vkd3d_free(sm4); return ret; } + if (shader_desc->is_dxil) + { + WARN("Shader is DXBC_DXIL.\n"); + return VKD3D_ERROR_INVALID_ARGUMENT; + } if (!shader_sm4_init(sm4, shader_desc->byte_code, shader_desc->byte_code_size, compile_info->source_name, &shader_desc->output_signature, message_context))
I'd argue we shouldn't necessarily reject shaders containing DXIL chunks in vkd3d_shader_sm4_parser_create(), as long as they contain a SHDR/SHEX chunk. That does imply shdr_handler() can't work as-is. We could e.g. initialise "desc->is_dxil" before calling parse_dxbc() in shader_extract_from_dxbc(), and then accept either DXIL or SHDR/SHEX chunks in shdr_handler() depending on whether "is_dxil" is set. Alternatively, we could just use separate handlers for SHDR/SHEX and DXIL, and avoid the "is_dxil" field in struct vkd3d_shader_desc.
+static inline unsigned int shader_sm6_read_uint32(struct vkd3d_shader_parser *parser) +{ + if (shader_sm6_is_end(parser)) + { + parser->failed = true; + return 0; + } + return *parser->ptr++; +}
- "inline" is probably superfluous above.
- This should just return an uint32_t. That applies to e.g. shader_sm6_read_bits(), as well.
+static bool shader_sm6_record_handle_blockinfo(struct vkd3d_shader_parser *parser, + struct vkd3d_shader_sm6_record *record) +{ + struct vkd3d_shader_sm6_parser *sm6 = vkd3d_shader_sm6_parser(parser); + struct vkd3d_shader_sm6_block *block = sm6->current_block; ... +}
This is called shader_sm6_record_handle_blockinfo(), but it actually operates on a block. There's also no reason to pass the parser as argument here.
+static bool shader_sm6_block_read(struct vkd3d_shader_sm6_block *parent, struct vkd3d_shader_parser *parser) +{ + struct vkd3d_shader_sm6_parser *sm6 = vkd3d_shader_sm6_parser(parser); + unsigned int reserve = (parent->id == MODULE_BLOCK) ? 12 : 2; + struct vkd3d_shader_sm6_block *block; + + sm6->current_block = parent; + + do + { + unsigned int abbrev_id = shader_sm6_read_bits(parser, parent->abbrev_len); + + switch (abbrev_id) + { + case END_BLOCK: + shader_sm6_align_32(parser); + return true; + + case ENTER_SUBBLOCK: + if (!(block = vkd3d_malloc(sizeof(*block))) || !vkd3d_array_reserve((void **)&parent->child_blocks, + &parent->child_block_capacity, max(reserve, parent->child_block_count + 1), + sizeof(*parent->child_blocks))) + { + ERR("Failed to allocate block.\n"); + return false; + } + + if (!shader_sm6_block_init(block, parent, parser)) + return false; + + parent->child_blocks[parent->child_block_count++] = block; + sm6->current_block = parent; + break; + + case DEFINE_ABBREV: + if (!shader_sm6_add_block_abbrev(parser)) + return false; + break; + + case UNABBREV_RECORD: + if (!shader_sm6_read_unabbrev_record(parser)) + { + FIXME("Failed to read unabbreviated record.\n"); + return false; + } + break; + + default: + if (!shader_sm6_read_abbrev_record(parser, abbrev_id - 4)) + { + FIXME("Failed to read abbreviated record.\n"); + return false; + } + break; + } + } while (!parser->failed); + + return false; +}
This seems like a nice place to split things; we could just error out on unrecognised abbreviation IDs, and then introduce the code to handle them one by one. That would allow a bunch of code for handling abbreviations and abbreviated records to just be deferred to later patches, and allow us to focus on the basic structure of the DXIL frontend in the initial patches.
@@ -1504,6 +1541,14 @@ const enum vkd3d_shader_target_type *vkd3d_shader_get_supported_target_types( VKD3D_SHADER_TARGET_D3D_ASM, }; + static const enum vkd3d_shader_target_type dxbc_dxil_types[] = + { + VKD3D_SHADER_TARGET_SPIRV_BINARY, +#ifdef HAVE_SPIRV_TOOLS + VKD3D_SHADER_TARGET_SPIRV_TEXT, +#endif + }; + TRACE("source_type %#x, count %p.\n", source_type, count); switch (source_type)
I imagine it will be a while until we can actually generate SPIR-V. The first step should probably be getting the disassembler; that would mean VKD3D_SHADER_TARGET_D3D_ASM here, or perhaps more likely a new VKD3D_SHADER_TARGET_DXIL_ASM.
@@ -1257,6 +1264,8 @@ static inline void *vkd3d_find_struct_(const struct vkd3d_struct *chain, #define TAG_SHEX VKD3D_MAKE_TAG('S', 'H', 'E', 'X') #define TAG_TEXT VKD3D_MAKE_TAG('T', 'E', 'X', 'T') +#define BITCODE_MAGIC 0xdec04342 +
That's "VKD3D_MAKE_TAG('B', 'C', 0xc0, 0xde)", right? Also, I don't think there should ever be a reason for this to be visible outside of dxil.c.
About patch 2/2:
It doesn't necessarily need to be addressed in this patch, but note that the autodetection code (i.e., the "if (options.source_type == VKD3D_SHADER_SOURCE_NONE)" block in main()) currently defaults to VKD3D_SHADER_SOURCE_DXBC_TPF for DXBC inputs. It might be nice to detect DXIL there as well. That probably doesn't require a full DXBC parser; iterating over the chunks in a DXBC isn't too complicated.
added 4 commits
Toggle commit list- Resolved by Giovanni Mascellani
Marvin replied on the mailing list:
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125697 Your paranoid android. === debian11 (build log) === Task: Patch failed to apply === debian11b (build log) === Task: Patch failed to apply
sm6.h is only used by dxil.c. Perhaps that will change in future commits, but until it does, its contents simply belong in dxil.c.
There's a good chance we'll want to emit sm6 from the HLSL compiler. Why not be proactive, and avoid the code churn?
The practical answer to that question is "to make the life of your reviewers easier and have your patches go in smoothly". I.e., you're essentially asking a reviewer to take a guess at potential future uses for these, or look at later patches in the series. This could be mitigated by writing a good commit message, of course, but even in that case you may end up in a discussion about future usage which would otherwise not be a concern for the patch in question.
To that point, it's not clear to me that we'd want a sm6.h for outputting sm6 from the HLSL compiler either; I think the code for that should just go into dxil.c as well.
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani