spirv-fuzz: add tests for full coverage of TransformationAccessChain

[sliu-UIUC]

This PR will fix #4286.

Update:
Fixed redundant code. Improved comments and styles. Achieved 100% coverage.

[afd]

Thanks - this looks good! A few comments about your comments :)

I agree with your analysis that the if condition can never be true.

I suggest we do a few changes to make the code clearer.

First, since GetIndexValue is only called on structs, let's change its name to GetStructIndexValue

Second, let's change its comment (in the header file) to reflect this, e.g. remove:

  // - the object being indexed is not a composite type

and start the whole comment with:

  // Requires that |object_type_id| is a struct type

Add an assertion at the top of the function to check that this is true:

assert(ir_context->get_def_use_mgr()->GetDef(object_type_id) == SpvOpTypeStruct && "Precondition: the type must be a struct type.");

And finally, change the redundant if condition into an assertion:

assert(spvOpcodeIsConstant(index_instruction->opcode()) && "A non-constant index should already have been rejected.");

In the process of investigating this I wrote the test below. Even though it doesn't increase coverage I think we may as well include it. So can you please add it to your PR?

Thanks!

TEST(TransformationAccessChainTest, StructIndexMustBeConstant) {
  std::string shader = R"(
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %4 "main"
               OpExecutionMode %4 OriginUpperLeft
               OpSource ESSL 320
          %2 = OpTypeVoid
          %3 = OpTypeFunction %2
          %6 = OpTypeInt 32 1
         %20 = OpUndef %6
          %7 = OpTypeStruct %6 %6
          %8 = OpTypePointer Function %7
         %10 = OpConstant %6 0
         %11 = OpConstant %6 2
         %12 = OpTypePointer Function %6
          %4 = OpFunction %2 None %3
          %5 = OpLabel
          %9 = OpVariable %8 Function
               OpReturn
               OpFunctionEnd
  )";

  const auto env = SPV_ENV_UNIVERSAL_1_4;
  const auto consumer = nullptr;
  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
  spvtools::ValidatorOptions validator_options;
  ASSERT_TRUE(fuzzerutil::IsValidAndWellFormed(context.get(), validator_options,
                                               kConsoleMessageConsumer));
  TransformationContext transformation_context(
      MakeUnique<FactManager>(context.get()), validator_options);
  // Bad: %9 is a pointer to a struct, but %20 is not a constant.
  ASSERT_FALSE(TransformationAccessChain(
                   100, 9, {20}, MakeInstructionDescriptor(9, SpvOpReturn, 0))
                   .IsApplicable(context.get(), transformation_context));
}

[afd]

Approved with one nit about an unnecessary whitespace change. Please revert that, and then we can run the CI bots and merge.

[afd]

The debug builds fail due to a comparison between a pointer and an enum value.

[sliu-UIUC]

test/fuzz/fuzzerutil_test.cpp can be ignored for this PR as it is implemented in another MaybeTest PR. Sorry for including it.

[afd]

Thanks for the fix! The only thing is that you don't have a new line at the end of the file.

[afd]

LGTM!