From fa834dea408935c7e86ee985bef24dbf964962b7 Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Thu, 16 Mar 2017 16:13:47 -0400
Subject: [PATCH] Fix validator message for bad logical pointer

Affects OpLoad and OpStore validation.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/588
---
 source/validate_id.cpp   |  4 ++--
 test/val/val_id_test.cpp | 62 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/source/validate_id.cpp b/source/validate_id.cpp
index dd799bc..039c9eb 100644
--- a/source/validate_id.cpp
+++ b/source/validate_id.cpp
@@ -1121,7 +1121,7 @@ bool idUsage::isValid<SpvOpLoad>(const spv_instruction_t* inst,
   if (!pointer || (addressingModel == SpvAddressingModelLogical &&
                    !spvOpcodeReturnsLogicalPointer(pointer->opcode()))) {
     DIAG(pointerIndex) << "OpLoad Pointer <id> '" << inst->words[pointerIndex]
-                       << "' is not a pointer.";
+                       << "' is not a logical pointer.";
     return false;
   }
   auto pointerType = module_.FindDef(pointer->type_id());
@@ -1150,7 +1150,7 @@ bool idUsage::isValid<SpvOpStore>(const spv_instruction_t* inst,
   if (!pointer || (addressingModel == SpvAddressingModelLogical &&
                    !spvOpcodeReturnsLogicalPointer(pointer->opcode()))) {
     DIAG(pointerIndex) << "OpStore Pointer <id> '" << inst->words[pointerIndex]
-                       << "' is not a pointer.";
+                       << "' is not a logical pointer.";
     return false;
   }
   auto pointerType = module_.FindDef(pointer->type_id());
diff --git a/test/val/val_id_test.cpp b/test/val/val_id_test.cpp
index e08c916..754420a 100644
--- a/test/val/val_id_test.cpp
+++ b/test/val/val_id_test.cpp
@@ -1824,23 +1824,50 @@ TEST_F(ValidateIdWithMessage, OpLoadResultTypeBad) {
               HasSubstr("OpLoad Result Type <id> '3' does not match Pointer "
                         "<id> '5's type."));
 }
+
 TEST_F(ValidateIdWithMessage, OpLoadPointerBad) {
   string spirv = kGLSL450MemoryModel + R"(
 %1 = OpTypeVoid
 %2 = OpTypeInt 32 0
-%9 = OpTypeFloat 32
 %3 = OpTypePointer UniformConstant %2
 %4 = OpTypeFunction %1
-%6 = OpFunction %1 None %4
-%7 = OpLabel
-%8 = OpLoad %9 %3
+%5 = OpFunction %1 None %4
+%6 = OpLabel
+%7 = OpLoad %2 %8
      OpReturn
      OpFunctionEnd
 )";
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  // Prove that SSA checks trigger for a bad Id value.
+  // The next test case show the not-a-logical-pointer case.
   EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("OpLoad Pointer <id> '4' is not a pointer."));
+              HasSubstr("ID 8 has not been defined"));
+}
+
+TEST_F(ValidateIdWithMessage, OpLoadLogicalPointerBad) {
+  string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypeFloat 32
+%4 = OpTypePointer UniformConstant %2
+%5 = OpTypePointer UniformConstant %3
+%6 = OpTypeFunction %1
+%7 = OpFunction %1 None %6
+%8 = OpLabel
+%9 = OpBitcast %5 %4 ; Not valid in logical addressing
+%10 = OpLoad %3 %9 ; Should trigger message
+     OpReturn
+     OpFunctionEnd
+)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  // Once we start checking bitcasts, we might catch that
+  // as the error first, instead of catching it here.
+  // I don't know if it's possible to generate a bad case
+  // if/when the validator is complete.
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("OpLoad Pointer <id> '9' is not a logical pointer."));
 }
 
 TEST_F(ValidateIdWithMessage, OpStoreGood) {
@@ -1875,8 +1902,31 @@ TEST_F(ValidateIdWithMessage, OpStorePointerBad) {
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("OpStore Pointer <id> '3' is not a pointer."));
+              HasSubstr("OpStore Pointer <id> '3' is not a logical pointer."));
+}
+
+TEST_F(ValidateIdWithMessage, OpStoreLogicalPointerBad) {
+  string spirv = kGLSL450MemoryModel + R"(
+%1 = OpTypeVoid
+%2 = OpTypeInt 32 0
+%3 = OpTypeFloat 32
+%4 = OpTypePointer UniformConstant %2
+%5 = OpTypePointer UniformConstant %3
+%6 = OpTypeFunction %1
+%7 = OpConstantNull %5
+%8 = OpFunction %1 None %6
+%9 = OpLabel
+%10 = OpBitcast %5 %4 ; Not valid in logical addressing
+%11 = OpStore %10 %7 ; Should trigger message
+     OpReturn
+     OpFunctionEnd
+)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("OpStore Pointer <id> '10' is not a logical pointer."));
 }
+
 TEST_F(ValidateIdWithMessage, OpStoreObjectGood) {
   string spirv = kGLSL450MemoryModel + R"(
 %1 = OpTypeVoid
-- 
2.7.4