GPU-AV: crash in Instruction::Describe() when an error record instruction offset is not an instruction boundary

[apollo-2006]

Environment:

• OS: Arch Linux, kernel 7.2.3
• GPU and driver version: AMD Radeon RX 9070 XT (Navi 48, gfx1201), RADV, Mesa 26.2.2
• SDK or header version if building from repo: built from main at 1710dfc (the lines cited below are
  unchanged on main as of c717893)
• Options enabled: GPU-AV (VK_LAYER_GPUAV_ENABLE=1)

Describe the Issue

GPU-AV can crash while formatting an error message, after validation has already done its job. The
offending instruction offset that GPU-AV writes into the error record is read back on the host and used
to decode a SPIR-V instruction, but nothing checks that the offset actually names one.

Building the layer with -D_GLIBCXX_ASSERTIONS and running llama.cpp's test-backend-ops under GPU-AV
aborts in Instruction::Describe():

/usr/include/c++/16/bits/stl_vector.h:1372: ... back() [with _Tp = OperandKind]: Assertion '!this->empty()' failed.

#5  spirv::Instruction::Describe() at layers/state_tracker/shader_instruction.cpp:125
        info = {types = std::vector of length 0, capacity 0}
        remaining_words = 4294967295
#6  spirv::FindShaderSource (instructions = std::vector of length 18478,
                             instruction_position_offset = 7406)
        at layers/error_message/spirv_logging.cpp:482
#7  SharedMemoryDataRace-RaceOnLoadStoreVsAtomic error logger (collide_inst_offset = 7406)
#8  gpuav::LogInstrumentationError
#9  gpuav::CommandBufferSubState::OnCompletion, QueueSubState::Retire, vvl::Queue::ThreadFunc

The mechanism, reading the frame locals:

1. Offset 7406 is inside the 18478 word module, so the existing
   collide_inst_offset < instrumented_shader->original_spirv.size() check at
   shared_memory_data_race.cpp:109 passes. The offset is in range but does not sit on an instruction
   boundary.
2. Describe() computes remaining_words = length - operand_offset, which underflows to 0xFFFFFFFF
   because the word at that offset has a length field too small to be an instruction header.
3. The opcode decoded from that word is not in the generated grammar table, so GetOperandInfo() returns
   the OpNop fallback at spirv_grammar_helper.cpp:2874, whose types vector is empty, and
   info.types.back() on line 125 is then undefined behaviour.

A release build without libstdc++ assertions does not necessarily fault, which is probably why this has
gone unnoticed. Forcing a deliberately bad offset into the same path on a release build gives a plain
SIGSEGV in Describe() on the queue retire thread.

Describe() is only reached when the module has no OpLine or NonSemantic DebugInfo, so
GetDebugLineOffset() returns 0 and FindShaderSource() takes the "Unable to find shader source" branch
at spirv_logging.cpp:478. Shaders compiled without debug info, which is how llama.cpp ships its Vulkan
shaders, are the ones that reach it.

How often the offset is bad

I patched the data race error logger to walk the module and report whether each offset landed on an
instruction boundary, then ran one full test-backend-ops sweep. Excluding the 0 and 131071 values that
the pass uses as "not recorded" markers, 2001 shadow word offsets reached the logger:

result	count
valid instruction boundary	1986
outside the module, caught by the existing size check	14
inside the module but mid instruction	1

So roughly 1 in 2000, which is why it takes a long run to hit and why it looked state dependent at first.

Some of the bad shadow words look like float payloads rather than packed offsets, for example 0x3f800000
(1.0f) and 0xc44b8000 (-814.0f). I could not establish why, and it may be a separate question about the
pass rather than about the logging, but it suggests the slot was clobbered rather than merely stale.

Expected behavior

An offset written by an instrumented shader is data the host cannot trust. If it does not name a real
instruction, the message should degrade, for example by saying the instruction was not recorded, rather
than taking the process down inside the validation layer.

Repro steps

1. Build the layer from main with -D_GLIBCXX_ASSERTIONS -g.
2. Build llama.cpp with the Vulkan backend and run test-backend-ops test with
   VK_LAYER_GPUAV_ENABLE=1 and GGML_VK_DISABLE_COOPMAT=1.
3. It aborts somewhere in the FLASH_ATTN_EXT range once a SharedMemoryDataRace error is reported with a
   bad shadow offset.

This is not a one command repro. The bad offset appears about once in 2000 reported offsets, so a full
sweep is needed, and it will not reproduce on a build without libstdc++ assertions even when the bad
offset occurs.

Possible fix

I have a candidate, 41 lines, if it is useful:

• an IsInstructionBoundary() helper in spirv_logging.cpp that walks the module and accepts the offset
  only if it lands exactly on an instruction start whose length fits in what remains, used by all three
  call sites that take an offset from an error record (gpuav_shader_instrumentor.cpp:2197,
  buffer_device_address.cpp:68, shared_memory_data_race.cpp:109, the last of which currently has a
  size check only). It runs only while building an error message.
• guards in Describe() for the empty operand list and for the length < operand_offset underflow.

With it, the same assertions build runs 18759 of 18759 backend tests clean while still reporting 15 data
race errors, and prints (specific instruction not recorded) for the 2 bad offsets it rejected in that
run. The full VVL test suite shows no change against baseline on this machine.

What I do not have is a positive test. The trigger is a shadow word written by the GPU, and I could not
find a way to forge one from the test framework. If you have a preferred way to test this kind of path, I
am happy to write it that way.

[spencer-lunarg]

1. Thanks for the report
2. Do you have the SPIR-V that caused the issue by chance? That is the fastest way for me to build a test, reproduce and fix these

[apollo-2006]

Following up with more evidence. The bad offset is not just a stale value: the shadow word itself is being read from outside the shadow array, and that has a worse consequence than the crash.

The corrupt shadow words are the bad offsets. 0xc44b8000 is -814.0f as a float. Unpacked with the pass's own layout, (0xc44b8000 >> 12) & 0x1FFFF = 17592, which is exactly the mid-instruction offset from the earlier diagnostic run. So the offsets that miss an instruction boundary are float payloads sitting where a packed shadow word should be.

The layer is reading past the end of shadow[]. I allocated 256 guard slots past slot_count_ in SharedMemoryDataRacePass::PostProcess and had inst_init_shadow fill them with 0xDEAD0000. If the application were spilling into shadow memory, the float payloads would survive. Re-running the same sweep:

	before	with guard band
float payloads	14	0
0xDEAD0000	n/a	54

So the out-of-range access is the instrumentation's own, not the app writing into the shadow array.

The offending indices. Adding if (start + idx >= length) to inst_do_store / inst_do_load (with the real length stashed in a shared variable by inst_init_shadow) and reporting the index gives slot indices 336, 338, 339, 340, 341 and 346. The host names the same variable every time, ID 2238. On the shader layout I believe this to be, that variable owns 120 slots, so the index is running well past its range rather than by one.

Consequence beyond the abort. shadow[start + idx] has no bounds check anywhere in shared_memory_data_race.comp, so an out-of-range read returns whatever LDS follows the array and the result is interpreted as a prior access. 0xDEAD0000 decodes to STORE_BIT | ATOMIC_BIT with thread_id 0. That means these reads can also produce false positive data race reports, which seems more serious than the crash, since a wrong validation error sends someone hunting a race that does not exist.

Trigger. test-backend-ops -o FLASH_ATTN_EXT from llama.cpp on RADV / RX 9070 XT, with GGML_VK_DISABLE_COOPMAT=1, so this is the plain load/store path and not the coopmat one. All 54 hits are head size 72 with a quantized K/V type (q5_0, q8_0, q5_1, q4_1, q4_0) and never f16, f32 or bf16. 5292 of 5292 backend tests still pass, which is why it goes unnoticed.

What I have not established is why the index goes out of range. Two theories are already dead: the app is not spilling into the shadow array (the guard band rules that out), and it is not the tmpsh sizing in llama.cpp's flash_attn.comp, whose subgroup reduction sites are all guarded by the row_split == 1 case that sizes the array to match. The remaining idea I have not tested is meta.element_count: the pass marks that many consecutive slots from start + idx, and when an access chain stops short of a scalar (a whole vector, struct or sub-array) that count can be large enough to run past the variable's range. You will likely recognise the cause faster than I will find it by probing.

Happy to keep digging, and the harness patches are small if any of them are useful to you. The host-side fix I offered above is still worth having regardless, since the offset comes from the shader and cannot be trusted, but it treats the symptom and this looks like the cause.

[spencer-lunarg]

Build llama.cpp with the Vulkan backend and run test-backend-ops test with VK_LAYER_GPUAV_ENABLE=1 and GGML_VK_DISABLE_COOPMAT=1.

if you could grab that SPIR-V, I can do this, but my machine might not support the COOPMAT operations to reproduce

there is also a VK_LAYER_GPUAV_DEBUG_DUMP_INSTRUMENTED_SHADERS=1 option you can use and GPU-AV will dump the shaders out for you

[spencer-lunarg]

PRs are welcomed, main thing is a test to ensure its a real bug (I believe you, but still) and also regression testing

[apollo-2006]

Edit: the specialization constant explanation below is wrong, see the correction further down. Leaving it here for the record, the SPIR-V links are still good.

Got it, and the dump option found it quickly. Note coopmat is not involved: GGML_VK_DISABLE_COOPMAT=1 is in the repro, so this is the plain load/store path and your machine should be fine.

SPIR-V here: https://gist.github.com/apollo-2006/5a98bdc16658165c7063cddfd847139e

• flash_attn_hsk72_q5_1_original.spv is the app shader as handed to vkCreateShaderModule (18944 words, passes spirv-val).
• flash_attn_hsk72_q5_1_instrumented.spv is what GPU-AV produced from it.
• .spvasm is the disassembly for convenience.

Four shader modules erred in the run and all four dumps are byte identical, so this single module is the whole repro. The variable the error names, %2238, is in its entry point interface.

While packaging it I think I found the cause, and it is not the logging.

%2238 is an array of a struct, and its length is a specialization constant expression:

%2235 = OpSpecConstantOp %uint IMul %406 %1903
%_arr__struct_2234_2235 = OpTypeArray %_struct_2234 %2235
%2238 = OpVariable %_ptr_Workgroup__arr__struct_2234_2235 Workgroup

In TypeManager, the array length is resolved as:

} else if (spv_type == SpvType::kArray) {
    const Constant* count = type_manager.FindConstantById(inst.Word(3));
    assert(count && !count->is_spec_constant_);
    m.array.length = count->GetValueUint32();
}

and Constant::GetValueUint32() asserts OpConstant then returns inst_.Word(3). For an OpSpecConstantOp, word 3 is the opcode being applied, not a value, so array.length becomes that opcode number. Both asserts are compiled out in release, which is what I was running.

SharedMemoryDataRacePass sizes the shadow array from GetScalarElementCount(), which multiplies by array.length, and the access chain offset it emits is computed from the real indices. So the slot range and the index disagree, shadow[start + idx] goes outside the array, and the word that comes back is whatever is next in LDS. That is the float payload, and one of those decoded to the mid instruction offset that aborted Describe().

This module has 10 array types whose length is a spec constant expression, and only the quantized variants of this shader use the struct arrays, which matches the trigger being quantized K/V types only and never f16/f32/bf16.

I did not find anything that resolves spec constants before instrumentation, so I think the assert is describing an input the pass can actually receive rather than one it cannot.

That also suggests the fix is bigger than the bounds check I offered: sizing has to handle a spec constant length, or the pass has to skip variables whose length it cannot evaluate. The host side hardening still seems worth having on its own, since the offset arrives from the shader either way. Happy to take a run at whichever shape you prefer, and I can rerun anything on RADV here.

[apollo-2006]

Correction to my last comment: the specialization constant theory is wrong, please disregard that part.

I checked it properly by printing what TypeManager actually resolves. Across 17692 array types in that run, every one came back as OpConstant with is_spec_constant_ false, so the length is resolved correctly and GetValueUint32() never sees an OpSpecConstantOp. I had read the dumped before module, where the lengths are still specialization constant expressions, and assumed the pass saw the same thing. It does not, they are folded by the time instrumentation runs.

What still holds is the measurement rather than the explanation. The instrumentation reads outside shadow[]: 256 guard slots past slot_count_ filled with a magic value caught 54 hits, and the float payloads vanished in the same run. The out of range slot indices are 336 to 346, against a slot_count of 335. All 54 hits are head size 72 with a quantized K/V type (q4_0, q4_1, q5_0, q5_1, q8_0) and never f16, f32 or bf16.

One thing I should not have asserted: which shared variable the out of range access belongs to. The error messages in that run all name %2238, but they are ordinary races logged earlier and are not the out of range accesses, which produced no message of their own. The offending indices are past the end of the whole slot range, so they are outside every variable including that one. I re-ran with the slot base recorded rather than guessing, and the answer is in the next comment.

So the out of bounds access is real, I just do not yet know why the index exceeds the range.

Sorry for the mistake 😅

[apollo-2006]

Found it, and your slot arithmetic is correct: GPU-AV is faithfully instrumenting an application access that is itself out of bounds.

The variable, measured this time rather than guessed: every report gives var_id=2238 with slot_base 27 and 28. That is Qf in llama.cpp's flash_attn.comp, which the pass sized at 120 slots.

The application bug is in the quantized path of the Q load loop:

const bool is_in_bounds = r < Br && d < HSK / 4 && i * Br + r < N;
#ifndef MMQ
        if (is_in_bounds) { Qf[r * qf_stride + d] = ...; }   // guarded
#else
        const uint buf_ib = r * qf_stride + d / 8;
        Qf[buf_ib].qs[buf_iqs] = ...;                        // not guarded
        Qf[buf_ib].ds = ...;                                 // not guarded

r can exceed Br. The non quantized branch discards those threads, the quantized one does not, so they write past the end of Qf.

Real specialization values for the failing pipeline: WorkGroupSize=256, Br=4, HSK=96, qf_stride=3, so Qf is 12 elements, 120 scalars. r = tid/24 reaches 10, which puts buf_ib at 30 or 31. The probe recorded writes at scalar offsets 309, 311 to 314, and 319, exactly elements 30 and 31. I will report that side to llama.cpp separately.

Only head size 72 was observed. The same pattern looks like it should affect other head sizes, but the pass instruments 91 of 647 pipelines in this run, so I am only claiming what was measured.

What is left on your side is that shadow[start + idx] is unchecked, so an application out of bounds access makes the layer read outside its own array and interpret whatever LDS follows as a prior access. That is where both the false race and the Describe() crash come from.

It also makes your test ask tractable: a shader that indexes a shared array out of range with a dynamic index reproduces the layer side deterministically, with no dependence on a GPU written shadow word. Happy to put up the PR with the bounds check plus a positive and a regression test if that shape works for you.

[apollo-2006]

Filed the application side as ggml-org/llama.cpp#29039, so that half is tracked.

[spencer-lunarg]

Honestly at this point, if you don't have a test, I'll be ok, just leave a comment (to this issue) with the bound check and we can call it a day (thanks again for all the work here!)

[apollo-2006]

Here is the bounds check. It sits in shared_memory_data_race.comp. The helpers get a slot index derived from the application's own access chain indices, so when the application indexes a shared array out of bounds the slot is out of bounds too, and they have no way to see the real array length because shadow[] is resized at link time. So inst_init_shadow records the length it is already given and every access tests against it.

@@ -77,6 +77,12 @@ const uint INST_OFFSET_MASK  = kSharedMemoryDataRace_InstOffsetMask;   // bits 2
 // This array has a placeholder size and the whole array is replaced in LinkFunctions
 shared uint shadow[99999];
 
+// The instrumented shader computes slot indices from its own access chain indices, so an
+// application that indexes a shared array out of bounds produces an out of bounds slot too.
+// shadow[] is sized to the real slot count at link time, which the shader cannot see, so
+// inst_init_shadow records it and every access checks against it.
+shared uint shadow_slot_count;
+
 // Pack a shadow word from its three fields.
 #define PACK_SHADOW(flags, thread_id, inst_offset) \
     ((flags) | (((inst_offset) << INST_OFFSET_SHIFT) & INST_OFFSET_MASK) | ((thread_id) & THREAD_ID_MASK))
@@ -93,6 +99,9 @@ shared uint shadow[99999];
 
 void inst_init_shadow(const in uint length, const in uint wg_size) {
     barrier();
+    if (gl_LocalInvocationIndex == 0) {
+        shadow_slot_count = length;
+    }
     for (uint i = gl_LocalInvocationIndex; i < length; i += wg_size) {
         shadow[i] = SENTINEL;
     }
@@ -100,6 +109,9 @@ void inst_init_shadow(const in uint length, const in uint wg_size) {
 }
 
 void inst_do_store(const in uint start, const in uint idx, const in uint inst_offset, const in uint variable_idx) {
+    if (start + idx >= shadow_slot_count) {
+        return;  // application indexed its shared memory out of bounds
+    }
     uint thread_id = gl_LocalInvocationIndex;
     uint ret = atomicExchange(shadow[start + idx], PACK_SHADOW(STORE_BIT, thread_id, inst_offset));
     // Not a race: initial SENTINEL, same thread's prior load, same thread's prior store.
@@ -112,6 +124,9 @@ void inst_do_store(const in uint start, const in uint idx, const in uint inst_of
 }
 
 void inst_do_load(const in uint start, const in uint idx, const in uint inst_offset, const in uint variable_idx) {
+    if (start + idx >= shadow_slot_count) {
+        return;  // application indexed its shared memory out of bounds
+    }
     uint thread_id = gl_LocalInvocationIndex;
 
     uint ret = atomicLoad(shadow[start + idx], gl_ScopeDevice, 0, 0);
@@ -149,6 +164,9 @@ void inst_do_load(const in uint start, const in uint idx, const in uint inst_off
 }
 
 void inst_do_atomic(const in uint start, const in uint idx, const in uint inst_offset, const in uint variable_idx) {
+    if (start + idx >= shadow_slot_count) {
+        return;  // application indexed its shared memory out of bounds
+    }
     uint thread_id = gl_LocalInvocationIndex;
     // Multiple atomics on the same slot don't race, so the offending thread isn't worth
     // tracking - write THREAD_ID_MASK ("unknown") in the tid field. inst_offset is still
@@ -171,6 +189,9 @@ void inst_do_coopmat_load(const in uint start, const in uint idx, const in uint
         // Treat coopmat load as multi_load with no known thread id. atomicOr can't write
         // inst_offset, so the other half reported by a later racing access is whatever
         // populated the slot first (or "not recorded" if the slot was still SENTINEL).
+        if (start + idx + loc >= shadow_slot_count) {
+            continue;  // application indexed its shared memory out of bounds
+        }
         uint ret = atomicOr(shadow[start + idx + loc], MULTI_LOAD_BIT);
         if ((ret & (STORE_BIT | ATOMIC_BIT)) != 0) {
             REPORT_RACE(kErrorSubCode_SharedMemoryDataRace_RaceOnLoad, inst_offset, thread_id, ret, variable_idx);
@@ -190,6 +211,9 @@ void inst_do_coopmat_store(const in uint start, const in uint idx, const in uint
 
         // No meaningful thread_id (write THREAD_ID_MASK = "unknown"), but record inst_offset
         // so races against this coopmat store can cite the coopmat instruction's source line.
+        if (start + idx + loc >= shadow_slot_count) {
+            continue;  // application indexed its shared memory out of bounds
+        }
         uint ret = atomicExchange(shadow[start + idx + loc], PACK_SHADOW(STORE_BIT, THREAD_ID_MASK, inst_offset));
         // The memory can't have been written or read, even by other coopmatload/store calls,
         // because the mapping of matrix elements to invocations is unknown.

Verified on RADV / RX 9070 XT with test-backend-ops -o FLASH_ATTN_EXT, same build either way, only this file changed:

	without	with
shadow words read from outside the array	10	0
data race reports	13	10
backend tests	5292/5292	5292/5292

Ten out of range reads become none, and three reports go with them. I did not map those three to individual reads, but no report can now cite a word from outside the array, and the other ten are unchanged. --gtest_filter='*SharedMemoryDataRace*': 71 passed, 6 skipped, 0 failed, the skips being the NV_cooperative_matrix2 and Workgroup variants this driver does not support.

One thing I did not fix, since it is your call: the extra shared uint is not accounted for in the size check at shared_memory_data_race_pass.cpp:364, which compares shared_memory_size + slot_count_ * sizeof(uint32_t) against maxComputeSharedMemorySize. It is 4 bytes, but a shader sitting exactly on the limit would now be instrumented when it should bail.

[spencer-lunarg]

I guess I'm a bit confused why doing the bound check in the glsl solves this? I feel this is masking the real error?

spirv::FindShaderSource (which it crashed in) is our logic after where we take the original SPIR-V and find debug source information to print source lines for you

So the shared_memory_data_race.comp compiled into SPIR-V offline and we "instrument" (inject) it into the app's shader

so the real "bound" check should be inside spirv_logging.cpp when we are trying to scan the original (non-instrumented) shader

[spencer-lunarg]

will note the FindShaderSource we pass in

const uint32_t collide_inst_offset = (raced_against >> inst_offset_shift) & inst_offset_bits;

the idea is we will have a SPIR-V with lets say 10,000 instructions, we need to know which instruction index caused the issue

so still trying to figure out if we are returning a invalid collide_inst_offset and that is causing the issue

I'm also trying to grasp why start + idx would ever be out as this almost feels like we are not setting the shadow[] length correctly then

[apollo-2006]

You're right, dropping the glsl check.

On why start + idx goes out of range: the length is correct. The application indexes its own shared array out of bounds, and the slot index is computed from the application's own access chain index, so an out of bounds access becomes an out of bounds slot. That is why collide_inst_offset is garbage, the word it was unpacked from was read past the end of shadow[].

Here is the check in spirv_logging.cpp:

@@ -467,11 +467,39 @@ static uint32_t GetDebugLineOffset(const std::vector<uint32_t>& instructions, ui
     return last_line_inst_offset;
 }
 
+// The instruction offset in an error record is written by the instrumented shader, so it is untrusted: it can be
+// stale or garbled and still land inside the module. Walking the module is the only way to know it names a real
+// instruction, and this only runs while building an error message.
+static bool IsInstructionBoundary(const std::vector<uint32_t>& instructions, uint32_t instruction_position_offset) {
+    if (instruction_position_offset < kModuleStartingOffset || instruction_position_offset >= instructions.size()) {
+        return false;
+    }
+    uint32_t offset = kModuleStartingOffset;
+    while (offset < instructions.size()) {
+        if (offset == instruction_position_offset) {
+            const uint32_t length = Length(instructions[offset]);
+            // the instruction has to fit in what is left of the module
+            return length != 0 && (instructions.size() - offset) >= length;
+        }
+        const uint32_t length = Length(instructions[offset]);
+        if (length == 0) {
+            return false;  // malformed, stop rather than spin
+        }
+        offset += length;
+    }
+    return false;
+}
+
 // There are 2 ways to inject source into a shader:
 // 1. The "old" way using OpLine/OpSource
 // 2. The "new" way using NonSemantic Shader DebugInfo
 void FindShaderSource(std::ostringstream& ss, const std::vector<uint32_t>& instructions, uint32_t instruction_position_offset,
                       bool debug_printf_only) {
+    // The offset comes from the instrumented shader, so treat it as untrusted.
+    if (instruction_position_offset != 0 && !IsInstructionBoundary(instructions, instruction_position_offset)) {
+        ss << "(specific instruction not recorded)\n";
+        return;
+    }
     const uint32_t last_line_offset = GetDebugLineOffset(instructions, instruction_position_offset);
     if (last_line_offset != 0) {
         Instruction last_line_inst(instructions.data() + last_line_offset);
@@ -531,6 +559,10 @@ void FindGlobalName(std::ostringstream& ss, const std::vector<uint32_t>& instruc
 // Tries to do the same logic as BufferDeviceAddressPass::RequiresInstrumentation
 // This is heavily favored for GLSL where this problem occurs
 void FindOpStructFromBDA(std::ostringstream& ss, const std::vector<uint32_t>& instructions, uint32_t instruction_position_offset) {
+    // The offset comes from the instrumented shader, so treat it as untrusted.
+    if (!IsInstructionBoundary(instructions, instruction_position_offset)) {
+        return;
+    }
     uint32_t last_seen_function_offset = 0;
 
     uint32_t offset = kModuleStartingOffset;
diff --git a/layers/state_tracker/shader_instruction.cpp b/layers/state_tracker/shader_instruction.cpp
index 5f6989d..034ab61 100644
--- a/layers/state_tracker/shader_instruction.cpp
+++ b/layers/state_tracker/shader_instruction.cpp
@@ -120,6 +120,15 @@ std::string Instruction::Describe() const {
     } else {
         const OperandInfo& info = GetOperandInfo(opcode);
         const uint32_t operands = static_cast<uint32_t>(info.types.size());
+        // An opcode the grammar table doesn't know falls back to OpNop, which has no operands to describe
+        if (operands == 0) {
+            return ss.str();
+        }
+        // length is the word count from the instruction header; if it can't even cover the
+        // result type / result id already printed, this is not an instruction boundary
+        if (length < operand_offset) {
+            return ss.str();
+        }
         const uint32_t remaining_words = length - operand_offset;
         for (uint32_t i = 0; i < remaining_words; i++) {
             OperandKind kind = (i < operands) ? info.types[i] : info.types.back();

Forcing instruction_position_offset to a mid instruction offset inside the module, assertions build, only these two files differing: without it exit=134 on the back() [with _Tp = OperandKind] assertion, with it exit=0 and "(specific instruction not recorded)". Applies cleanly to main. *SharedMemoryDataRace*: 71 passed, 6 skipped, 0 failed.

FindShaderSource takes the offset as a parameter, so a module plus an offset that is not an instruction boundary is all a test needs, no GPU involved.

[spencer-lunarg]

this is better, but Instruction::Describe() should have an assert, not an if check... if it is invalid by that time, we are only hiding the real bug

So I think FindShaderSource should have some check and return something, but again, its hiding something, why is the instruction_position_offset passed in not valid?!?!

We get that from

#define PACK_SHADOW(flags, thread_id, inst_offset) \
    ((flags) | (((inst_offset) << INST_OFFSET_SHIFT) & INST_OFFSET_MASK) | ((thread_id) & THREAD_ID_MASK))

here the inst_offset is packed into the 32-bit word and

const uint32_t collide_inst_offset = (raced_against >> inst_offset_shift) & inst_offset_bits;

pulls it back out and passes it into FindShaderSource

so in your test that is crashing, what is this inst_offset being set to

Is the instrumentation setting it incorrectly?

[apollo-2006]

The instrumentation is packing it correctly. In that run 1986 of the reports came back with inst_offset 4092, which is a real instruction boundary, and the thread id varying underneath it. The 10 bad ones decode from words like 0xc44b8000 and 0x3f800000, which are -814.0f and 1.0f, so those words are application data and were never written by PACK_SHADOW at all. The out of range read is where they come from, not the packing.

Agreed on the assert, changed:

@@ -120,6 +120,11 @@ std::string Instruction::Describe() const {
     } else {
         const OperandInfo& info = GetOperandInfo(opcode);
         const uint32_t operands = static_cast<uint32_t>(info.types.size());
+        // Callers validate that the offset names a real instruction before decoding it, so
+        // neither of these can happen for a well formed offset: an unknown opcode falls back
+        // to OpNop and has no operands, and length always covers what has already been printed.
+        assert(operands != 0);
+        assert(length >= operand_offset);
         const uint32_t remaining_words = length - operand_offset;
         for (uint32_t i = 0; i < remaining_words; i++) {
             OperandKind kind = (i < operands) ? info.types[i] : info.types.back();

FindShaderSource keeps the check and returns the message, so a bad offset cannot reach Describe() and those asserts hold. Re-ran the forced mid instruction offset on the assertions build: exit=0, no assert fires, "(specific instruction not recorded)" instead. Full diff of both files applies cleanly to main.