This is an archive of the discontinued LLVM Phabricator instance.

[X86] Don't clobber reserved registers with stack adjustments
ClosedPublic

Authored by dotdash on Nov 4 2017, 9:37 PM.

Details

Summary

Calls using invoke in funclet based functions are assumed to clobber
all registers, which causes the stack adjustment using pops to consider
all registers not defined by the call to be undefined, which can
unfortunately include the base pointer, if one is needed.

To prevent this (and possibly other hazards), skip reserved registers
when looking for candidate registers.

This fixes issue #45034 in the Rust compiler.

Diff Detail

Repository
rL LLVM

Event Timeline

dotdash created this revision.Nov 4 2017, 9:37 PM
mkuper added a reviewer: rnk.Nov 6 2017, 1:57 PM
mkuper edited edge metadata.
mkuper added a subscriber: rnk.

I'm not sure whether it's the right fix.
I would expect the clobbers list of an instruction to be precise - if it says it's clobbered, you can't rely on it being preserved across the call, whatever the circumstances. So maybe the right thing to do would be to filter reserved registers out of the clobber list for these invokes when they're lowered?

@rnk, thoughts?

rnk edited edge metadata.Nov 6 2017, 2:08 PM

lgtm

I guess we were just getting lucky before. We could only go at most 4 deep into the GR32_NOREX_NOSP class, which is more or less:

def GR32 : RegisterClass<"X86", [i32], 32,
                         (add EAX, ECX, EDX, ESI, EDI, EBX, EBP, ESP,
                              R8D, R9D, R10D, R11D, R14D, R15D, R12D, R13D)>;

If we used EBX on Windows instead, we'd avoid this bug as well.

rnk accepted this revision.Nov 6 2017, 2:10 PM

Marking accepted in the review tool.

I'm not sure whether it's the right fix.
I would expect the clobbers list of an instruction to be precise - if it says it's clobbered, you can't rely on it being preserved across the call, whatever the circumstances. So maybe the right thing to do would be to filter reserved registers out of the clobber list for these invokes when they're lowered?

@rnk, thoughts?

Maybe SP is a special case, but CALL is definitely a def of SP, and we don't worry about it's liveness or preservation because it's reserved.

This revision is now accepted and ready to land.Nov 6 2017, 2:10 PM
This revision was automatically updated to reflect the committed changes.