[MS] Copy the symbols assigned to the former instruction when memory folding.

[pengfei]

Summary

The memory folding raplaced the old instruction without copying the symbols assigned. Which will resulted in built fail due to the lost symbols.

[pengfei]

Ping.

[pengfei]

Ping again. This is a bug fix. If no one opposes, I'll submit it to LLVM.

[craig.topper]

[craig.topper]

Can you pre-commit the test with the current codegen to show the problem this patch fixes?

[pengfei]

In D78471#2030719, @craig.topper wrote:

Can you pre-commit the test with the current codegen to show the problem this patch fixes?

Sure.

[pengfei]

Rebased.

[craig.topper]

Is the problem here really that foldMemoryOperand doesn't know to preserve the post instr symbol?

[pengfei]

In D78471#2032415, @craig.topper wrote:

Is the problem here really that foldMemoryOperand doesn't know to preserve the post instr symbol?

Yes and no. It's the problem I wanted to fix firstly. But after I read the code of hardening, I think the spill is also a problem. Because that pass always load the function address to register at the beginning. I think the spilling violates the objective the pass does. Beside, according to calling conversation, there are volatile registers before calling. So I think it's not a cost if we pin the address register.

[craig.topper]

In D78471#2033001, @pengfei wrote:

In D78471#2032415, @craig.topper wrote:

Is the problem here really that foldMemoryOperand doesn't know to preserve the post instr symbol?

Yes and no. It's the problem I wanted to fix firstly. But after I read the code of hardening, I think the spill is also a problem. Because that pass always load the function address to register at the beginning. I think the spilling violates the objective the pass does. Beside, according to calling conversation, there are volatile registers before calling. So I think it's not a cost if we pin the address register.

I think we should be calling NewMI->cloneInstrSymbols in the same spot we copy memory references in TargetInstrInfo::foldMemoryOperand. Or we shouldn't disable folding if any symbols are present. We're effectively cloning an instruction when we fold the memory and we should make sure we don't drop things when we do that.

Whether SLH should spill or not is something we can still address, but that's a detail of what the right thing to do for SLH is. I view it separately from incorrectly cloning the instruction in folding.

[chandlerc]

In D78471#2033046, @craig.topper wrote:

In D78471#2033001, @pengfei wrote:

In D78471#2032415, @craig.topper wrote:

Is the problem here really that foldMemoryOperand doesn't know to preserve the post instr symbol?

Yes and no. It's the problem I wanted to fix firstly. But after I read the code of hardening, I think the spill is also a problem. Because that pass always load the function address to register at the beginning. I think the spilling violates the objective the pass does. Beside, according to calling conversation, there are volatile registers before calling. So I think it's not a cost if we pin the address register.

I think we should be calling NewMI->cloneInstrSymbols in the same spot we copy memory references in TargetInstrInfo::foldMemoryOperand. Or we shouldn't disable folding if any symbols are present. We're effectively cloning an instruction when we fold the memory and we should make sure we don't drop things when we do that.

+1 -- I'd definitely prefer to *enable* the spill, but correctly handle symbols attached.

Whether SLH should spill or not is something we can still address, but that's a detail of what the right thing to do for SLH is. I view it separately from incorrectly cloning the instruction in folding.

Spills should be fine.

SLH very specifically runs *before* register allocation because hardening speculative *reloads* from spilled registers is not necessary in its security model. We don't harden other spills, and we don't need to avoid this one.

The reason the SLH pass forces the call target to be in a register (more specifically, a *virtual* register, because RA hasn't happened yet) is to ensure that the load and call are two separate instructions. When they're folded together, we don't have a chance to harden the target of the call *after* the load. All we need is to *unfold* this source load, we don't need to prevent further spills. It will spill the hardened value and reload it, which is within the security model of SLH.

[pengfei]

Copy the symbol only. Thanks @craig.topper 's suggestion and @chandlerc 's interpretation.

[craig.topper]

llvm/lib/CodeGen/TargetInstrInfo.cpp

599

Probably worth its is own comment and blank line between it and the memory operand handling.

[pengfei]

Address review comment.

[pengfei]

Rebase.

[craig.topper]

I lost track of this and didn't realize I never approved it.

LGTM

[pengfei]

Thanks @craig.topper

llvm/lib/CodeGen/TargetInstrInfo.cpp

 //===-- TargetInstrInfo.cpp - Target Instruction Information --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (NewMI) {
     assert((!(Flags & MachineMemOperand::MOLoad) ||
             NewMI->mayLoad()) &&
            "Folded a use to a non-load!");
     assert(MFI.getObjectOffset(FI) != -1);
     MachineMemOperand *MMO =
         MF.getMachineMemOperand(MachinePointerInfo::getFixedStack(MF, FI),
                                 Flags, MemSize, MFI.getObjectAlign(FI));
     NewMI->addMemOperand(MF, MMO);
 
+    // The pass "x86 speculative load hardening" always attaches symbols to
+    // call instructions. We need copy it form old instruction.
+    NewMI->cloneInstrSymbols(MF, MI);
+
     return NewMI;
   }
 
   // Straight COPY may fold as load/store.
   if (!MI.isCopy() || Ops.size() != 1)
     return nullptr;
 
   const TargetRegisterClass *RC = canFoldCopy(MI, Ops[0]);

llvm/test/CodeGen/X86/speculative-load-hardening-no-spill.ll

 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening | FileCheck %s
 
 define i32 @foo(void ()** %0) {
 ; CHECK-LABEL: foo:
-; CHECK-NOT:    .Lslh_ret_addr0:
 ; CHECK:         callq *(%{{.*}})
+; CHECK-NEXT:  .Lslh_ret_addr0:
 ; CHECK-NEXT:    movq %rsp, %rcx
 ; CHECK-NEXT:    movq -{{[0-9]+}}(%rsp), %rax
 ; CHECK-NEXT:    sarq $63, %rcx
 ; CHECK-NEXT:    cmpq $.Lslh_ret_addr0, %rax
   %2 = load void ()*, void ()** %0
   call void asm sideeffect "", "~{bx},~{cx},~{dx},~{bp},~{si},~{di},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"()
   call void %2()
   ret i32 0
 }