[CodeGen][X86] Improving robustness of PATCHABLE_OP wrapper instruction

[sylvain-audi]

When activating -fms-hotpatch in clang, the "patchable-function" pass replaces the first machine instruction with a wrapper PATCHABLE_OP.

This wrapping loses some information about the wrapped instruction: when a PATCHABLE_OP instruction is handled by X86AsmPrinter::emitInstruction, the wrapped instruction simply gets lowered without going through X86AsmPrinter::emitInstruction itself.

Here is an example in C/C++, where a tail call doesn't get lowered properly: https://godbolt.org/z/1Pjcbx87n

The source of the issue seems to be the loss of information in PatchableFunction::runOnMachineFunction when replacing a MachineInstr with the PATCHABLE_OP one: It only keeps the OpCode and operands of the wrapped instruction, and X86AsmPrinter::emitInstruction can't be called from it.

We are looking for a clean way to achieve this. Any suggestions?

[llvmbot]

@llvm/issue-subscribers-backend-x86

[tru]

Turns out that this is not only windows related. This happens for all X86 platforms when -fms-hotpatch is being used.

[tru]

Ping @aganea

[sylvain-audi]

Ping!

[tru]

Maybe @phoebewang or @RKSimon can help out here since it's very much a complicated X86 backend issue.

[phoebewang]

I'm not familiar with hotpatch. Is the patch https://reviews.llvm.org/D137642 will solve the problem here too?

[sylvain-audi]

I'm not familiar with hotpatch. Is the patch https://reviews.llvm.org/D137642 will solve the problem here too?

That patch fixes the selection of the function's instruction to make patchable (guarantee it's at least 2 bytes long or is preceded by a 2-byte nop).
This issue is about the lowering part, which I coudn't figure out how to do without losing information.

[sylvain-audi]

Ping!

I'm currently stuck, none of my attempts seem sustainable.
Currently in our fork we have a hack that simply inserts a 2-byte nop at the beginning of the function, disabling the opcode modification. This results is a lot of useless nops.

Note that MSVC seems to do this differently, as in MSVC blog: "most of the time, the compiler can juggle things so that you don’t even notice that it arranged for the first instruction of a function to be a multi-byte instruction. ". But it seems that doing that in clang could be pretty involved.

[phoebewang]

cc @KanRobert

[tru]

Ping on this - anyone know if someone that can help us progress on this?