Fix segfault due to b64 encoding

[abrown]

Prior to this patch, bconst.b64 encoded its instruction with a 32-bit immediate that caused improper decoding of the MOV instruction; fixes #911.

[awortman-fastly]

can we avoid the 64-bit mov entirely here? since the immediate is either 0 or 1, it doesn't seem like we need to specify a 64bit destination register, and just rely on zero-extension to do its thing. I think that means an equally correct fix would be just removing .rex().w() from the recipe?

[abrown]

@awortman-fastly, that makes sense to me and the test passes; let me try it that way and see if @bnjbvr has a strong opinion one way or another. If we don't want to use the 32-bit encoding we can revert 56f0733; otherwise we can squash when we merge.

[bnjbvr]

@awortman-fastly @abrown It's an elegant solution indeed! Unfortunately, since there's no REX variant with this solution, it means it restricts the possible output register to the 8 first registers in 64-bit mode. I think a proper fix is a mix of both: first the 8 bytes recipe with REX.W, and then after this the 32-bit immediate recipe. (REX must come first just so it's selected first by register allocation, with the fewer constraints on register assignment, and then the relaxation phase may try to use the 4 bytes recipe if that fits the operand constraints.)
Does it make sense?

[awortman-fastly]

Ah, I'd misunderstood how rex() and w() affect the recipe :) The W bit is really what I meant isn't necessary here, and thought that rex() got an accessor to that, rather than indicating that REX is an acceptable prefix for the recipe. Since r8d-r15d get the same zero-extension, we just need to not emit the W prefix right? f.ex 41b901000000 encodes mov r9d, 1, where the W really adds nothing extra (but more zeroes)

(sorry for the back and forth on your small change @abrown!)

[abrown]

I don't see an encoding for a REX + 8B /r in the Intel manual; I only see REX.W + 8B /r (for MOV r64,r/m64) and 8B /r (for MOV r32,r/m32). Leaving the W bit unset in the REX byte doesn't make sense to me because W means "64 bit operand size" and that is what we would be trying to do with that encoding; the non-REX encoding would try to do a 32-bit operand. But I may be missing something from your comment? Why avoid the W bit?

[bjorn3]

Why avoid the W bit?

So that the immediate can be 32bit. Implicit zero extension should take care of writing the right 64bit value into the register if I understand it correctly.

[abrown]

Yeah, I think in the case of just wanting a 32-bit operand that makes sense, but just encode it as B8 + rd id (I used the wrong one above)--no REX and therefore no W set. What I'm saying is that if we need a 64-bit version as @bnjbvr mentioned then it doesn't make sense to me to just have REX B8 + rd io--that doesn't exist in the Intel manual. It only makes sense to me to have REX.W B8 + rd io.

[abrown]

I do see this comment "Use of the REX.R prefix permits access to additional registers (R8-R15)" so perhaps that is what @awortman-fastly meant?

[edit: never mind, I think that only applies to ModR/M bytes, which the immediate version of MOV is not using]

[awortman-fastly]

I think the manual only lists REX.W + XX encodings to indicate that 64bit operands are valid for the instruction (eg REX.W + 88 isn't present) - the lack of a pairing there doesn't mean that REX.B B8 would be forbidden.

Section 3.1.1.1, Opcode Column in the Instruction Summary Table (Instructions without VEX Prefix) is the one that applies here, since the encoding is B8 +rd id as noted in MOV encodings, where the table describes REX.B selecting R8-R15 in place of the lower general-purpose registers for +rd encoding instructions

edit: notably, B8 movs with REX, but not REX.W, have the B8 +rd id encoding. So 40b8000000 still encodes mov eax, 0 (and in fact is what we use for 32-bit immediates before instruction shrinking finds a smaller permissible encoding without REX)

edit2: said "postopt" in edit1, but meant "instruction shrinking" :)

[abrown]

I think I see what you mean; you're aiming to make more registers available, right? (Agreed that the instruction table may not have all pairings). I do see the following on Table B-15 (page 2590) and perhaps you are trying to use the second of these?

__MOV - Move Data__
...
| immediate to register | 0100 000B : 1100 011w : 11 000 reg : imm |
| immediate32 to qwordregister (zero extend) | 0100 100B 1100 0111 : 11 000 qwordreg : imm32 |
| immediate to register (alternate encoding) | 0100 000B : 1011 w reg : imm |
| immediate64 to qwordregister (alternate encoding) | 0100 100B 1011 1000 reg : imm64 |
...

However, I don't see a good way to set the B bit on the REX byte in cranelift (not that that can't be fixed).

[abrown]

Never mind, I just don't understand cranelift yet: I can see that the REX.B bit is correctly set for the current 64-bit immediate implementation.

[abrown]

I can also verify that when I try to encode bconst.b64 without the W bit set and a 32-bit boolean (i.e. e.enc64(bconst.bind(B64), rec_pu_id_bool.opcodes(vec![0xb8]).rex());) the run test still passes and I see an output encoding of 41 ba 00000001 in binemit.

[awortman-fastly]

REX.B is selected based on the operand register when the selected recipe is actually executed - that's handled by rex1 in pu_id_bool and the same elsewhere. rex2 would do the same REX bit selection as appropriate for ModRM-encoded operands, which is why it's not used here (but is elsewhere). So just .rex() is sufficient to indicate that a REX prefix is wanted with the recipe (I'm pretty sure? @bnjbvr definitely knows that better than me), and permits the possibility of REX.B, but REX alone has no effect on the 16/32/64bit opcodes.

re. which encoding line, I meant the third one in fact! The fourth is what we get with REX.W B8, and the third line as a 32-bit mov would have w=1 for 1011 1RRR or 0xb8 to 0xbf. REX.B would select the upper registers if rex1 is given a register that would necessitate the extra bit (r8-r15).

[abrown]

@awortman-fastly, thanks; I think we are on the same page now. I implemented exactly that for the comment above and the emitted 41 ba ... for register %r10 matches what you describe (REX.B == 0100 0001 and RRR == 010) . Let me push it and @bnjbvr can comment.

[bnjbvr]

Thank you both for the investigation and patch! Confirming that the REX.B bit is set automatically by the rex1 etc functions. Let's move the test to binary64 and add one that uses rax to make sure it works in both cases!