Check for valid SPIR-V <id>s by comparing against 0
This change consistently uses if(id != 0) { ... } syntax for choosing
code paths where the SPIR-V <id> is valid. This allows the value()
method to assert that we don't inadvertently use an <id> for an operand
which does not exist. This makes for an elegant syntax when parsing
SPIR-V instructions into structures which store <id>s for operands which
are optional, such as common in image instructions, while catching
mistakes centrally.
Bug: b/205642050
Change-Id: I3e1f1340a56bd3c848fdf3724088660046bf29ed
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/39268
Reviewed-by: Alexis Hétu <sugoi@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Pipeline/SpirvID.hpp b/src/Pipeline/SpirvID.hpp
index 67ca59d..e90a8cc 100644
--- a/src/Pipeline/SpirvID.hpp
+++ b/src/Pipeline/SpirvID.hpp
@@ -15,6 +15,8 @@
#ifndef sw_ID_hpp
#define sw_ID_hpp
+#include "System/Debug.hpp"
+
#include <cstdint>
#include <unordered_map>
@@ -31,15 +33,16 @@
public:
SpirvID() = default;
inline SpirvID(uint32_t id);
+
inline bool operator==(const SpirvID<T> &rhs) const;
inline bool operator!=(const SpirvID<T> &rhs) const;
inline bool operator<(const SpirvID<T> &rhs) const;
- // value returns the numerical value of the identifier.
+ // Returns the numerical value of the identifier.
inline uint32_t value() const;
private:
- uint32_t id = 0;
+ uint32_t id = 0; // Valid ids are in the range "0 < id < Bound".
};
template<typename T>
@@ -68,6 +71,10 @@
template<typename T>
uint32_t SpirvID<T>::value() const
{
+ // Assert that we don't attempt to use unassigned IDs.
+ // Use if(id != 0) { ... } to avoid invalid code paths.
+ ASSERT(id != 0);
+
return id;
}
diff --git a/src/Pipeline/SpirvShader.cpp b/src/Pipeline/SpirvShader.cpp
index c56fd38..6239a20 100644
--- a/src/Pipeline/SpirvShader.cpp
+++ b/src/Pipeline/SpirvShader.cpp
@@ -207,7 +207,7 @@
case spv::OpLabel:
{
- ASSERT(currentBlock.value() == 0);
+ ASSERT(currentBlock == 0);
currentBlock = Block::ID(insn.word(1));
blockStart = insn;
}
@@ -224,8 +224,8 @@
case spv::OpKill:
case spv::OpUnreachable:
{
- ASSERT(currentBlock.value() != 0);
- ASSERT(currentFunction.value() != 0);
+ ASSERT(currentBlock != 0);
+ ASSERT(currentFunction != 0);
auto blockEnd = insn;
blockEnd++;
diff --git a/src/Pipeline/SpirvShaderControlFlow.cpp b/src/Pipeline/SpirvShaderControlFlow.cpp
index 7893023..754e91d 100644
--- a/src/Pipeline/SpirvShaderControlFlow.cpp
+++ b/src/Pipeline/SpirvShaderControlFlow.cpp
@@ -778,6 +778,7 @@
<< "[label=\"M\" style=dashed color=blue]"
<< std::endl;
}
+
if(block.second.continueTarget != 0)
{
file << " block_" << block.first.value() << " -> "
diff --git a/src/Pipeline/SpirvShaderImage.cpp b/src/Pipeline/SpirvShaderImage.cpp
index d13ed5a..426b087 100644
--- a/src/Pipeline/SpirvShaderImage.cpp
+++ b/src/Pipeline/SpirvShaderImage.cpp
@@ -557,7 +557,7 @@
}
SIMD::Int n = 0;
- if(sampleId.value())
+ if(sampleId != 0)
{
Operand sample(this, state, sampleId);
if(!sample.isConstantZero())
@@ -587,7 +587,7 @@
oobMask |= As<SIMD::Int>(CmpNLT(As<SIMD::UInt>(w), SIMD::UInt(depth)));
}
- if(sampleId.value())
+ if(sampleId != 0)
{
Operand sample(this, state, sampleId);
if(!sample.isConstantZero())
