Fix Invalid Instruction format in fuzzgen

[afonso360]

👋 Hey,

So in #4733 oss-fuzz discovered that we have pretty much been using the wrong instruction formats for all opcodes for a while... (Thanks @jameysharp for digging into this!).

This PR does 2 things:

• Force fuzzgen to use the correct InstructionFormat for each opcode
• Add an assert to all InstructionFormat inserters to ensure that this does not happen again (even for other users of cranelift).

Fixes #4733
cc: @cfallin

[jameysharp]

I'm quite pleased with your fuzzgen changes here. I was afraid this would be much more complicated to fix!

@cfallin, what's your take on the new assert when constructing instructions? Is there ever a case where someone should be allowed to use a different instruction-format constructor than the standard format for an opcode?

[fitzgen]

Is there ever a case where someone should be allowed to use a different instruction-format constructor than the standard format for an opcode?

That should never happen. Would break our ISLE extractors for matching clif instructions, at the very least. But a ton of helpers only look for certain opcodes in certain formats, etc.

[jameysharp]

That should never happen. Would break our ISLE extractors for matching clif instructions, at the very least. But a ton of helpers only look for certain opcodes in certain formats, etc.

Just to be clear: If it should never happen, then a runtime assertion that it never happens is a good idea, right?

[fitzgen]

Yeah, probably a debug assertion. (I haven't looked at this patch at all)

[cfallin]

Indeed it should never happen, and this is usually enforced by the generated builder interface's per-instruction methods, but nothing currently stops someone from manually putting the wrong pieces together in the data structure or invoking the per-instruction-format method with an opcode as in the test here. That will break a lot of things (including ISLE matchers as Nick notes). Our best defense is to guard against this at construction time as best we can.

An assert in the per-instruction-format builder API seems totally reasonable, but let's make it a debug_assert rather than assert so that we don't have any runtime impact (the debug assert will still be tested in the fuzzing configuration).