This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pengfei on Apr 19 2020, 10:09 PM.

Details

Summary

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

Diff Detail

Event Timeline

pengfei created this revision.Apr 19 2020, 10:09 PM
efriedma edited reviewers, added: arsenm; removed: chandlerc.Apr 23 2020, 9:47 PM

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

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

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

Sure.

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

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.

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.

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 updated this revision to Diff 263640.May 13 2020, 1:05 AM

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

pengfei retitled this revision from [x86/SLH] Pin function address in physical register after it been hardened. to [MS] Copy the symbols assigned to the former instruction when memory folding..May 13 2020, 8:00 PM
pengfei edited the summary of this revision. (Show Details)
craig.topper added inline comments.May 16 2020, 2:58 PM
llvm/lib/CodeGen/TargetInstrInfo.cpp
599 ↗(On Diff #263640)

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

pengfei updated this revision to Diff 264566.May 18 2020, 2:26 AM

Address review comment.

craig.topper accepted this revision.Jun 10 2020, 12:21 AM

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

LGTM

This revision is now accepted and ready to land.Jun 10 2020, 12:21 AM
This revision was automatically updated to reflect the committed changes.