Conversation
| auto length = usedefs_.FindDef(inst->words[lengthIndex]); | ||
| if (!length.first || (SpvOpConstant != length.second.opcode && | ||
| SpvOpSpecConstant != length.second.opcode)) { | ||
| if (!length.first || !spvOpcodeIsConstant(length.second.opcode)) { |
There was a problem hiding this comment.
This is too permissive given the code that later checks the value.
The later code uses the lengths of the instructions and maps that to an assumption about it being a SpvOpConstant or SpvOpSpecConstant.
Should have a TODO saying the remaining cases should be handled (or punted entirely)
There was a problem hiding this comment.
That comment no longer applies now that the later code switches on opcode.
|
+1, but I don't quite understand the 'remaining cases' in David's comments :( |
|
Feedback addressed. |
source/validate_id.cpp
Outdated
| const bool is_signed = typeWords[3]; | ||
| if (width == 64) { | ||
| if (is_signed) { | ||
| int64_t value = constWords[3] | (int64_t{constWords[4]} << 32); |
There was a problem hiding this comment.
This doesn't compile with VS 2013:
error C2397: conversion from 'const unsigned int' to 'int64_t' requires a narrowing conversion
which I find a bit strange, since it's 32-bit to 64-bit, so there's clearly enough bits.
I'd change it to:
(uint64_t{constWords[4]} << 32)We're shifting left, so signedness makes no difference.
There was a problem hiding this comment.
Thanks @yavn. I changed it to uint64_t, as you suggest. Aside from the VS2013 error, I've realized that shifting a signed value runs into undefined behaviour all too easily.
Use uint64_t instead of int64_t to shift left a SPIR-V word representing high-order bits of a signed 64-bit value. That way shifting left by 32 bits is well defined.
source/validate_id.cpp
Outdated
| int64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); | ||
| return value > 0; | ||
| } else { | ||
| uint64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); |
There was a problem hiding this comment.
An unsigned value is non-zero if any of its bits are non-zero. So you could have saved some work here. Or you could have been a little more DRY by sharing this computation with the signed case, and then had the signed case do the constructor-cast.
|
-1 for invalid code for 48 bit widths, and missing test cases. |
|
Made |
| if (width > 32) { | ||
| // The spec currently doesn't allow integers wider than 64 bits. | ||
| const auto hiWord = constWords[4]; // Must exist, per spec. | ||
| if (is_signed && (hiWord >> 31)) return false; |
There was a problem hiding this comment.
This depends critically on the fact that hiWord is an unsigned type, and yet you declare it via auto.
Ok, I guess.
|
-1 |
|
Fixed |
|
+2 yay |
|
Squashed and merged to master via 3fb2676. |
@dneto0 @AWoloszyn @qining @antiagainst PTAL.