Page MenuHomePhabricator

[X86] Emit .cfi_escape GNU_ARGS_SIZE when adjusting the stack before calls
ClosedPublic

Authored by mkuper on Sep 24 2015, 6:10 AM.

Details

Summary

This should hopefully fix the exception handling issues in PR24792.

There are two things I'd appreciate an opinion on:

a) Does it make sense to expose it like this to FrameLowering, or should the interface be more abstract, and actually turned into a .cfi_escape somewhere lower?

b) I'm not sure how to optimize away consequent directives that provide the same size. I don't think this can be done in eliminateCallFramePseudoInstr(), because (1) it's not guaranteed to be called in program order (even though in practice I think that is what happens when we have pushes), and (2) I think the basic-block order can change later on.
It may make sense to eliminate the redundant directives somewhere in the MC layer, but I'm not sure where that can fit in. Any suggestions?

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 35616.Sep 24 2015, 6:10 AM
mkuper retitled this revision from to [X86] Emit .cfi_escape GNU_ARGS_SIZE when adjusting the stack before calls.
mkuper updated this object.
mkuper added reviewers: DavidKreitzer, rnk.
mkuper added a subscriber: llvm-commits.
rnk edited edge metadata.Sep 28 2015, 11:09 AM

a) Does it make sense to expose it like this to FrameLowering, or should the interface be more abstract, and actually turned into a .cfi_escape somewhere lower?

I'd make it more abstract internally. I'm thinking MCStreamer::EmitGnuArgsSize, and then the two streamer impls can add the 0x2e internally.

b) I'm not sure how to optimize away consequent directives that provide the same size. I don't think this can be done in eliminateCallFramePseudoInstr(), because (1) it's not guaranteed to be called in program order (even though in practice I think that is what happens when we have pushes), and (2) I think the basic-block order can change later on.
It may make sense to eliminate the redundant directives somewhere in the MC layer, but I'm not sure where that can fit in. Any suggestions?

I wouldn't worry about it now. If we keep it abstract, we can do the optimization as part of EmitCFIInstructions without digging into escaped CFI sequences.

lib/Target/X86/X86FrameLowering.cpp
2073 ↗(On Diff #35616)

We shouldn't emit this if we have an FP for the frame.

Thanks Reid,
I'll upload an updated patch with a cleaner interface.

lib/Target/X86/X86FrameLowering.cpp
2073 ↗(On Diff #35616)

That was my original thought too, but I talked about it with Dave, and he suggested that was incorrect.
E.g.:

while (...)

try {
   foo(...);
   ...
} catch (...) {
   ...
}

}

While all stack references will be fine without gnu_args_size (assuming FP), the stack may overflow due to the missing cleanups.

majnemer added inline comments.
lib/MC/MCAsmStreamer.cpp
1023 ↗(On Diff #35616)

!Values.empty()

lib/Target/X86/X86FrameLowering.cpp
2074 ↗(On Diff #35616)

Why not use DW_CFA_GNU_args_size ?

test/CodeGen/X86/push-cfi.ll
71 ↗(On Diff #35616)

"resest"

98 ↗(On Diff #35616)

resest

116 ↗(On Diff #35616)

actually efficient

Thanks, David!

lib/MC/MCAsmStreamer.cpp
1023 ↗(On Diff #35616)

Right, thanks!

lib/Target/X86/X86FrameLowering.cpp
2074 ↗(On Diff #35616)

No good reason, only noticed it exists after I uploaded the patch.

mkuper updated this revision to Diff 35940.Sep 29 2015, 12:11 AM
mkuper edited edge metadata.

Updated the interface.
I abused the "Offset" field of MCCFIInstruction for the size - but I think this actually makes sense. If anyone prefers a new field, let me know.

The optimization of removing duplicate .cfi_escapes will go into a separate commit, once I figure out where it fits in.

DavidKreitzer added inline comments.Sep 30 2015, 2:35 PM
lib/MC/MCAsmStreamer.cpp
1038 ↗(On Diff #35940)

Is there a reason you made Size signed? DW_CFA_GNU_args_size takes an unsigned value. (Same question for the other routines that take a signed Size.)

1041 ↗(On Diff #35940)

It would be safer to make Buffer a little bigger so that you could encode any possible uint64_t. I think the maximum ULEB encoding for a 64-bit integer would be 10 bytes?

lib/Target/X86/X86FrameLowering.cpp
2052 ↗(On Diff #35940)

I believe DW_CFA_GNU_args_size is only needed in routines that contain EH handlers. It is not needed for debugging, nor is it needed for routines that need unwind table entries but have no handlers. So I don't think this is quite the right condition. It will result in more directives than necessary.

Thanks, Dave!

lib/MC/MCAsmStreamer.cpp
1038 ↗(On Diff #35940)

For consistency with the rest of the interface, mostly.

1041 ↗(On Diff #35940)

Oh my. Of course!
I copied this from a different place that uses an 8-byte buffer, but there it's safer because it feeds an unsigned int directly into encodeULEB128()...

lib/Target/X86/X86FrameLowering.cpp
2052 ↗(On Diff #35940)

Makes sense. I'll refine the condition.

mkuper added inline comments.Oct 1 2015, 12:14 AM
lib/Target/X86/X86FrameLowering.cpp
2052 ↗(On Diff #35940)

Actually, if I understand things correctly now (and thanks for the patience!), we only need it before what was in IR an invoke, never before an IR call.
Is that right?

mkuper updated this revision to Diff 36229.Oct 1 2015, 5:23 AM

Fixed buffer overflow, and changed the condition to only add GNU_ARGS_SIZE if there are any landing pads in the function.
We can probably do better (by adding them only for invokes), but I'd rather leave that for a separate patch.

DavidKreitzer added inline comments.Oct 1 2015, 5:53 AM
lib/Target/X86/X86FrameLowering.cpp
2052 ↗(On Diff #36229)

That sounds right to me.

rnk accepted this revision.Oct 6 2015, 11:21 AM
rnk edited edge metadata.

lgtm

Thanks for the ping and the fix!

lib/Target/X86/X86FrameLowering.cpp
2068 ↗(On Diff #36229)

Hm, got it.

This revision is now accepted and ready to land.Oct 6 2015, 11:21 AM
This revision was automatically updated to reflect the committed changes.