This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][Fix] LdSt optimization generate premature stack-popping
ClosedPublic

Authored by dnsampaio on Mar 6 2020, 8:38 AM.

Details

Summary

When moving add and sub to memory operand instructions,
aarch64-ldst-opt would prematurally pop the stack pointer,
before memory instructions that do access the stack using
indirect loads.
e.g.

int foo(int offset){
    int local[4] = {0};
    return local[offset];
}

would generate:

sub     sp, sp, #16            ; Push the stack
mov     x8, sp                 ; Save stack in register
stp     xzr, xzr, [sp], #16    ; Zero initialize stack, and post-increment, making it invalid
------ If an exception goes here, the stack value might be corrupted
ldr     w0, [x8, w0, sxtw #2]  ; Access correct position, but it is not guarded by SP

Diff Detail

Event Timeline

dnsampaio created this revision.Mar 6 2020, 8:38 AM
foad resigned from this revision.Mar 6 2020, 8:51 AM

I think there's a secondary problem here: if we move an SP adjustment, we might also need to adjust the unwind/debug info. This is particularly nasty on Windows, where messing up the unwind info can actually cause a miscompile.

If you don't want to spend the time to try to figure that out, I'd be happy to just completely forbid optimizing sp adjustments here. It should be rare enough that it doesn't matter much in practice.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1832

I'm not sure what you think this mayAlias check is doing?

dnsampaio marked an inline comment as done.Mar 8 2020, 2:37 PM

I think there's a secondary problem here: if we move an SP adjustment, we might also need to adjust the unwind/debug info. This is particularly nasty on Windows, where messing up the unwind info can actually cause a miscompile.

If you don't want to spend the time to try to figure that out, I'd be happy to just completely forbid optimizing sp adjustments here. It should be rare enough that it doesn't matter much in practice.

Hi @efriedma, thanks for the review.
I would be glad in taking a look into the windows unwind issue, but I don't know anything about it. Could you give me some further details?
Perhaps we can just prevent any SP adjustment for the moment, until I have the windows fix in place.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1832

Following a similar code from MachineInstr.mayAlias code, I thought this was just eliminating some trivial cases where the instruction would not access the stack, but now that I see the definition, I'm not so sure.

dnsampaio updated this revision to Diff 249062.Mar 9 2020, 4:29 AM

Do not optimize post-increment SP in windows targets

clang-format-diff

The problem on Windows is that every instruction in the prologue/epilogue has an associated unwind opcode, represented as a pseudo-instruction. If those pseudo-instructions don't match the actual instructions, the offsets will all be wrong, so the unwind info will be corrupt. In practice, we saw a miscompile involving a call placed immediately after a ret instruction. The check should look something like https://github.com/llvm/llvm-project/blob/6d026c89dc6a3ba405e6767a3aa0bbb6ba27a0c9/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp#L1998 .

DWARF unwind doesn't work like that, of course. But you'll still get very weird results if you set a breakpoint in certain places. I'm pretty sure we have a pseudo-instruction for DWARF stack adjustments? I forget what that looks like.

dnsampaio updated this revision to Diff 249791.Mar 11 2020, 4:08 PM
  • Fixed tests
  • Avoid optimization when target is Windows with CFI, as we need to correctly update unwind information
efriedma added inline comments.Mar 11 2020, 4:38 PM
llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1832

I still don't know why you're calling PseudoSourceValue::mayAlias. You don't care whether the access may alias other accesses; you care whether you're accessing the stack.

dnsampaio updated this revision to Diff 249811.EditedMar 11 2020, 6:23 PM
  • Correct mayAlias to isStack

Despite the name, isStack() doesn't include all accesses to the stack; there's also FixedStack PseudoSourceValues.

That said, I'm not sure it's worth bothering; non-stack PseudoSourceValues are relatively rare, at least on AArch64, so you're not getting much benefit.

dnsampaio updated this revision to Diff 249894.Mar 12 2020, 4:27 AM
  • Do not use pseudo-value
efriedma added inline comments.Mar 12 2020, 9:05 AM
llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1802

"continue"? Should we really be continuing to search past an instruction which modifies sp?

dnsampaio updated this revision to Diff 250203.Mar 13 2020, 7:46 AM
  • Do not allow a instruction that may load or store between the LdSt[SP] and the SP update.
dnsampaio updated this revision to Diff 250211.Mar 13 2020, 8:04 AM
  • Removed unecessary changes
  • Removed incorrect spacing changes
This revision is now accepted and ready to land.Mar 13 2020, 2:25 PM
This revision was automatically updated to reflect the committed changes.