[val, opt] Disallow extra NSDI operands without option

[s-perron]

In previous commits, the validator was changed to allow an arbitrary number of
operands to NonSemantic.Shader.DebugInfo (NSDI) instructions. This was done to
allow the validator to handle versions of NSDI that it does not know about yet.

However, this can cause problems in a couple of ways:

1. The validator will not reject invalid instructions with incorrect operand
   counts for known versions of NSDI.
2. It can mask bugs in producers generating invalid debug info.

This commit enforces strict version and operand count checks by default for
known NSDI instructions, and introduces the --allow-unknown-nsdi-version
flag to spirv-val and spirv-opt to bypass these checks when working with
future NSDI versions.

[s-perron]

@dnovillo Do you have an opinions on this? I don't know your use case. I'm hoping using an option will not be a problem.

[alan-baker]

The intent is that future NSDI versions might introduce new operands to existing instructions (so long as not using them doesn't change meaning). So probably what should happen here is the strict parsing should happen only if the known NSDI version (from headers) is >= the parsed version. Then this option is instead auto set in a sense.

[s-perron]

I'd like the know the actual use case for considering an unreleased valid by default. The problem is that all tools (like spirv-opt) that rely on the validator to reject invalid code before running must be able to handle any possible instruction on any location. I don't know how often this will cause problems, but that is what concerns me.

I also don't know how consistent we are in what we allow or disallow. For example, we allow unknown extended NSDI instructions in section 9 of the module (with the types and constants), but not inside functions. I don't know if that is an oversight or if we can assume that no new NDSI instructions will be located inside a function definition.

[alan-baker]

I'd like the know the actual use case for considering an unreleased valid by default. The problem is that all tools (like spirv-opt) that rely on the validator to reject invalid code before running must be able to handle any possible instruction on any location. I don't know how often this will cause problems, but that is what concerns me.

It's not unreleased. It's meant for newer versions of NSDI to continue working with older version of tooling (e.g. in a driver).

I also don't know how consistent we are in what we allow or disallow. For example, we allow unknown extended NSDI instructions in section 9 of the module (with the types and constants), but not inside functions. I don't know if that is an oversight or if we can assume that no new NDSI instructions will be located inside a function definition.

That's probably an oversight.

[dnovillo]

The main problem I see with this approach is that it invalidates the behaviour we wanted to establish in KhronosGroup/SPIRV-Registry#369:

1. An old tool cannot read a newer module on its own. A tool shipped in a driver that knows only version 100 rejects a version 110 module by default.
2. Unless I'm mis-reading the code, a tool would not be able to be strict on known versions and lenient on unknown versions at the same time. The flag is one switch that turns off both checks together. A caller cannot keep operand checking for version 100 while accepting version 110.
3. An extra operand on a version-100 instruction in a version-100 module is a producer bug. The same extra operand in a version-110 module the tool does not know is a legitimate new field. This is what the versioning scheme wants to be able to distinguish.

[s-perron]

I tried to keep the existing behaviour with the flag. We can easily change that. The important part of the discussion is if it is reasonable for the validator to say that a version of NSDI is valid by default even if there is no definition for it. I don't think this was agreed on in the spir-v registry issue. I see

Tools that don't understand new instructions may reject the module outright or try to be flexible at parsing instructions that they may not understand.

Nothing specific about spir-v val. This option allows other tools that rely on the validator to filter out invalid spir-v to opt-in to the flexible parsing.

If we really need the validator to accept unknown version because . Then I'm okay with that. I want to make sure that is what we really want to do.