Do not panic when reference types found but safepoints not enabled

[csmoe]

Closes #1257
r? @bnjbvr

• This has been discussed in issue #1257
• A short description of what this does, why it is needed:
• This PR contains test cases, if meaningful.
• A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

[bnjbvr]

Thanks for giving it a try! We probably need to add an error to the current list, though, otherwise nothing will be reported, right?

Can you also add a test, please?

[csmoe]

@bnjbvr okay. would you mind reopening this PR? mindless close click.

[bnjbvr]

I can't, probably because the merge is a no-op; some commits should be added to the source branch, that is, csmoe:master.

[csmoe]

@bnjbvr sorry for a stupid question, how to add a test for this? cannot find testing-docs about deal with .wasm file.

[bnjbvr]

No worries, not a stupid question!

You can copy a test from the filetests/verifier directory, use a reference type instruction in there, and add a check line that ensures that an error is caught, like here: https://github.com/bytecodealliance/cranelift/blob/master/filetests/verifier/simd-lane-index.clif#L9

[csmoe]

@bnjbvr to reproduce this panic, I had to insert test safepoint statement, should I place this test into filetests/safepoint? Or just leave it inside filetests/verifier?

test safepoint
target x86_64

function %test(i32, r64, r64) -> r64 {
    ebb0(v0: i32, v1:r64, v2:r64):
        jump ebb1(v0)
    ebb1(v3: i32):
        v4 = irsub_imm v3, 1
        jump ebb2(v4)
    ebb2(v5: i32):
        resumable_trap interrupt
        brz v5, ebb1(v5)
        jump ebb3
    ebb3:
        v6 = null.r64
        v7 = is_null v6
        brnz v7, ebb2(v0)
        jump ebb4
    ebb4:
        brnz v0, ebb5
        jump ebb6
    ebb5:
        return v1
    ebb6:
        return v2
}

[bnjbvr]

Ah interesting! It appears that the check for safepoints being enabled is not in the verifier where it really ought to be, since this error should be caught as soon and as explicitly as possible. Would you be interested in moving the check in there, instead?

[csmoe]

@bnjbvr okay, I'll move it.
btw, cranelift seems planning to improve the docs https://github.com/bytecodealliance/cranelift/issues/1265 , is there any chance for me to contribute from a know-nothing-about-wasm-cranelift perspective? the benefit I can imagine is making the docs more friendly to beginners but might annoy your seniors with lots of "stupid" questions during writing.

[bnjbvr]

Is there any chance for me to contribute from a know-nothing-about-wasm-cranelift perspective?

Sure, please do! It's a great way to ensure documentation is best and up to date. We'll soon have an official communication channel for instant messaging, for quicker feedback, in the meanwhile we can answer here, or on Mozilla's IRC (https://wiki.mozilla.org/IRC, channel #cranelift where my nick is bbouvier).