Page MenuHomePhabricator

[X86] Add X86FixupSeparateStack pass
Needs ReviewPublic

Authored by mlemay-intel on Feb 10 2016, 1:25 PM.

Details

Summary

The X86FixupSeparateStack pass traverses the graph of basic blocks for
a function and tracks which physical registers derive from the stack
register (to point at stack data) at each instruction. It also
modifies each instruction that uses one of those registers as the base
register in a memory operand, unless the register is ESP or EBP. This
is to direct those memory accesses to the stack segment. It also
modifies each instruction that uses EBP as the base register in a
memory operand if EBP does not derive from the stack register at that
instruction. This is to direct those memory accesses to the data
segment.

This pass is enabled with the +separate-stack-seg option.

Event Timeline

mlemay-intel retitled this revision from to [X86] Add X86FixupSeparateStack pass.
mlemay-intel updated this object.
eugenis edited edge metadata.Feb 10 2016, 4:49 PM

I may be able to look at this next week, but it is a bit outside of my area of expertise. Maybe find another reviewer?

Also, this needs tests. A lot of tests.

I may be able to look at this next week, but it is a bit outside of my area of expertise. Maybe find another reviewer?

Thank you. I added you as a reviewer to all of these since they are all related to SafeStack, but I'll also look for another reviewer as you suggested.

Also, this needs tests. A lot of tests.

I haven't yet found any existing tests in LLVM for a pass like this that I can use as an example, but I'll keep looking. The challenge in writing a test for this pass that fits in the LLVM testing framework is that this pass modifies particular instructions depending on which registers get allocated as base registers for the instructions' memory operands, etc. When a test is written as IR, I don't know of a way to control which registers get selected.

mlemay-intel added inline comments.Feb 18 2016, 7:06 AM
lib/Target/X86/X86FixupSeparateStack.cpp
627

Perhaps this should be a warning instead of a debug message. Are there examples of passes that issue warnings for similar things?

delena added a subscriber: delena.Feb 28 2016, 11:38 PM
delena added inline comments.
lib/Target/X86/X86FixupSeparateStack.cpp
594

Are you working with load? I see PUSH32r and MRMDestMem - DestMem looks like store

597

New line after )

600

Why masked instructions are out of scope?
DestMem can't be VEX_4V.

611

Add assert (MemoryOperand != -1)

639

if (!Op.isReg() || !Op.isDef())

continue;
662

Do you need to delete SuccReqs?

678

Do you check this when you add the pass?

682

Who deletes the object created by "new" ?

lib/Target/X86/X86TargetMachine.cpp
397

for 32-bit only?

craig.topper added inline comments.
lib/Target/X86/X86.td
275

Can this be done with a command line switch in X86TargetMachine.cpp like VZeroUpper? I don't see that this needs to be a feature bit.

lib/Target/X86/X86FixupSeparateStack.cpp
600

DestMem can be VEX_4V. We check for it in both MCCodeEmitter and the Disassembler. I believe at least vmaskmovpd uses it.

Sorry for my delayed response. Activity notifications have not been reaching my inbox.

lib/Target/X86/X86.td
275

Your suggestion should work fine at the LLVM level, but then how would you suggest passing the flag to Clang?

lib/Target/X86/X86FixupSeparateStack.cpp
594

Loads are handled as well on line 632.

597

Will do. Thanks.

600

I assume that VEX-encoded instructions are not used for storing the stack pointer. I think that handling them would increase the complexity of line 610 for computing the operand number of the base register (I looked at X86MCCodeEmitter::EmitVEXOpcodePrefix to understand how these instructions are represented in LLVM). Thus, I skip them to avoid that extra complexity. I'll add a comment to note this decision.

611

X86II::getMemoryOperandNo always returns 0 for instructions of this form. I'll add a comment noting this assumption.

639

Good catch!

662

Ownership of the pointer is assigned to a shared_ptr in processBasicBlock. However, I will revise processBasicBlock to accept a shared_ptr parameter so that the pointer ownership is more clear.

678

I think it may make sense to keep this check here so that an error message is emitted if the associated command line option is specified for an unsupported target.

682

This pointer is assigned to a shared_ptr in processBasicBlock.

Expand comments, streamline code, and clarify object ownership.

Fix bug that occurred when cycle was formed with
AddrRegReqs::addPredecessor. AddrRegReqs::useInMemOp could modify
DenseSet being iterated in addPredecessor.

Track registers pointing into SS segment across some spills and fills.

Detect some instructions emitted for variadic argument intrinsics that
store SS pointers, avoiding assertion failures for those instructions.

Add option to control level of verification that is applied to
instructions that store SS pointers.

More extensive support for tracking spills and fills of registers containing stack pointers across basic blocks.
Recognize additional instructions that are used to derive stack pointers.

Corrected/updated some comments. Added a check that stack pointer spill requirements are satisfied.

Rebase.
Revise StackPtrSpillReqs::addPredecessor to correctly handle the case when Demand is empty.
Revise comments and fix whitespace.

Fixed a bug in how segment switching instructions were inserted around string instructions.
Made comment indentation more consistent.
Added support for tracking the flow of stack pointer values through instructions with multiple register inputs (e.g. CMOV instructions).
Enhanced spill/fill tracking to support functions with eliminated frame pointers.
Selected more efficient data structures.
Revised AddrRegReqs::derive to reflect the fact that stack pointer values do not flow through the flags register.

Avoids erasing previous derivation data when a register is both used and defined in an instruction.
Ignores implicit and undef registers when tracking derivation.
Removes code and associated options for tracking variadic argument pointers.
Eliminates assert on storing stack pointers to memory.

boazo added a reviewer: zvi.Oct 5 2016, 3:32 AM
zvi edited edge metadata.Oct 7 2016, 10:50 PM

I may be able to look at this next week, but it is a bit outside of my area of expertise. Maybe find another reviewer?

Thank you. I added you as a reviewer to all of these since they are all related to SafeStack, but I'll also look for another reviewer as you suggested.

Also, this needs tests. A lot of tests.

I haven't yet found any existing tests in LLVM for a pass like this that I can use as an example, but I'll keep looking. The challenge in writing a test for this pass that fits in the LLVM testing framework is that this pass modifies particular instructions depending on which registers get allocated as base registers for the instructions' memory operands, etc. When a test is written as IR, I don't know of a way to control which registers get selected.

Now that tests were added to this patch in .ll form, would it be better to change them to machine IR (.mir) form?

In D17095#565252, @zvi wrote:

I may be able to look at this next week, but it is a bit outside of my area of expertise. Maybe find another reviewer?

Thank you. I added you as a reviewer to all of these since they are all related to SafeStack, but I'll also look for another reviewer as you suggested.

Also, this needs tests. A lot of tests.

I haven't yet found any existing tests in LLVM for a pass like this that I can use as an example, but I'll keep looking. The challenge in writing a test for this pass that fits in the LLVM testing framework is that this pass modifies particular instructions depending on which registers get allocated as base registers for the instructions' memory operands, etc. When a test is written as IR, I don't know of a way to control which registers get selected.

Now that tests were added to this patch in .ll form, would it be better to change them to machine IR (.mir) form?

Yes, that's exactly the type of testing infrastructure I was looking for, but I didn't know it existed until you pointed it out. Thanks. I'll update this patch.

mlemay-intel edited edge metadata.

Add new pass to registry.
Convert tests to MIR form.
Update tests to reflect previous removal of assertion for certain stores of stack pointers.

Added -sep-stk-seg-flat-mem-func option to specify functions that use a flat memory model.
Fixed bug in StackPtrSpillReqs::supply to prevent coalescing of ranges that need to remain separate.

Revised comments.
Converted a condition into an assertion in AddrRegReqs::lookupRoots.

Mark EAX as not being derived from any possible stack pointer registers after calls.

mkuper resigned from this revision.Jan 30 2017, 4:24 PM