Skip to content
Snippets Groups Projects

vkd3d-shader/dxil: Read the DXIL module.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:dxil_module into master

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
  • So far I just managed to review the first commit. Will continue later.

  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit e1d5e5f7
  • 1305 for (i = 0; i < block->child_block_count; ++i)
    1306 value_count = sm6_parser_compute_max_value_count(sm6, block->child_blocks[i], value_count);
    1307
    1308 switch (block->id)
    1309 {
    1310 case FUNCTION_BLOCK:
    1311 sm6->value_capacity = max(sm6->value_capacity, value_count);
    1312 /* The value count returns to its previous value after handling a function. */
    1313 value_count = old_value_count;
    1314 break;
    1315 default:
    1316 break;
    1317 }
    1318
    1319 return value_count;
    1320 }
    • Comment on lines +1297 to +1320

      I don't understand what this function is supposed to calculate. At least for the moment, unless I'm missing something, the number of values that get allocated should exactly be the number of MODULE_CODE_FUNCTION records in the root block: why can't you just compute that thing and need something more complicated?

      Relatedly, and even more if we need this complicated function to decide how many values we need to store, maybe sm6_parser_get_current_value() could assert() that we're not overflowing.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • Conor McCarthy added 6 commits

    added 6 commits

    • 920b530b - vkd3d-shader/dxil: Read the type table.
    • b239968c - vkd3d-shader/dxil: Read the value symbol table.
    • c67d9e16 - vkd3d-shader/dxil: Validate the module format version.
    • 08633ad9 - vkd3d-shader/dxil: Read global function declarations.
    • 2501855f - vkd3d-shader/dxil: Read numeric constants.
    • 58eb3d1c - vkd3d-shader/dxil: Read function bodies.

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit c67d9e16
  • 1186
    1187 for (i = 0; i < block->record_count; ++i)
    1188 {
    1189 record = block->records[i];
    1190 switch (record->code)
    1191 {
    1192 case MODULE_CODE_FUNCTION:
    1193 FIXME("Functions are not implemented yet.\n");
    1194 break;
    1195
    1196 case MODULE_CODE_GLOBALVAR:
    1197 FIXME("Global variables are not implemented yet.\n");
    1198 break;
    1199
    1200 case MODULE_CODE_VERSION:
    1201 if (!record->operand_count)
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 2501855f
  • 1532 enum vkd3d_shader_register_type reg_type = VKD3DSPR_INVALID;
    1533 const struct sm6_type *type, *elem_type;
    1534 enum vkd3d_data_type reg_data_type;
    1535 const struct dxil_record *record;
    1536 unsigned int i, value_idx;
    1537 struct sm6_value *dst;
    1538 uint64_t value;
    1539
    1540 for (i = 0, type = NULL; i < block->record_count; ++i)
    1541 {
    1542 record = block->records[i];
    1543 value_idx = sm6->value_count;
    1544
    1545 if (record->code == CST_CODE_SETTYPE)
    1546 {
    1547 if (!record->operand_count)
  • Giovanni Mascellani approved this merge request

    approved this merge request

  • From the point of view of security (i.e., rejecting adversarial input), there are still some holes. For example, all additions in sm6_parser_compute_max_value_count() and related helpers should be checked against overflowing.

  • Author Developer

    It's not possible for a DXBC chuck to contain enough records to cause overflow in value_count. I would add a check anyway if it was reasonably simple, but it's a bit messy. EDIT: the compression likely makes it possible so I'll see what can be done.

    Edited by Conor McCarthy
  • Conor McCarthy added 4 commits

    added 4 commits

    • a0ec8322 - vkd3d-shader/dxil: Validate the module format version.
    • ceb8fc5b - vkd3d-shader/dxil: Read global function declarations.
    • 3a63a2a2 - vkd3d-shader/dxil: Read numeric constants.
    • 1a1b5630 - vkd3d-shader/dxil: Read function bodies.

    Compare with previous version

  • Conor McCarthy added 7 commits

    added 7 commits

    • f4eda564 - vkd3d-shader/dxil: Use size_t where applicable.
    • 8330d35e - vkd3d-shader/dxil: Read the type table.
    • 21e50ce3 - vkd3d-shader/dxil: Read the value symbol table.
    • a88e9f68 - vkd3d-shader/dxil: Validate the module format version.
    • bc94c2ab - vkd3d-shader/dxil: Read global function declarations.
    • dc70c57c - vkd3d-shader/dxil: Read numeric constants.
    • f7a9e309 - vkd3d-shader/dxil: Read function bodies.

    Compare with previous version

    • +#define TYPE_RECORD_VALIDATE_OPERAND_MIN_COUNT(min_count) do {\
      +    if (record->operand_count < (min_count))\
      +    {\
      +        WARN("Invalid operand count %u for type code %u, id %zu.\n", record->operand_count, record->code, type_index);\
      +        return VKD3D_ERROR_INVALID_SHADER;\
      +    } } while (false)
      +
      +#define TYPE_RECORD_VALIDATE_OPERAND_MAX_COUNT(max_count) do {\
      +    if (record->operand_count > (max_count))\
      +        WARN("Ignoring %u extra operands for type code %u, id %zu.\n", record->operand_count - (max_count),\
      +                record->code, type_index); } while (false)
      +
      +#define TYPE_RECORD_VALIDATE_OPERAND_COUNT(min_count, max_count) do {\
      +    TYPE_RECORD_VALIDATE_OPERAND_MIN_COUNT(min_count);\
      +    TYPE_RECORD_VALIDATE_OPERAND_MAX_COUNT(max_count); } while (false)

      Do we really need these to be macros? It seems like the only reason these aren't regular functions is the usage of WARN, and I'm inclined to say we should be using something built on top of vkd3d_shader_verror()/vkd3d_shader_vwarning() anyway, much like e.g. hlsl_error() and hlsl_warning(). Not just here, of course.

      I do want to resolve error reporting/handling sooner rather than later. While to some extent that's something that can be tweaked later case-by-case, it's also something that both impacts the basic structure of the parser, and something that's generally settled for the other vkd3d-shader parsers.

    • Author Developer

      I declared sm6_parser_get_type() with a const char *function parameter and use a sm6_parser_get_type macro to insert __FUNCTION__ into each call. Is that an acceptable approach generally? Validating operand counts would be neater if done that way.

    • Please register or sign in to reply
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading