[PATCH] D13132: [X86] Emit .cfi_escape GNU_ARGS_SIZE when adjusting the stack before calls

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, Dave!


================
Comment at: lib/MC/MCAsmStreamer.cpp:1038
@@ +1037,3 @@
+
+void MCAsmStreamer::EmitCFIGnuArgsSize(int64_t Size) {
+  MCStreamer::EmitCFIGnuArgsSize(Size);
----------------
DavidKreitzer wrote:
> 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.)
For consistency with the rest of the interface, mostly.

================
Comment at: lib/MC/MCAsmStreamer.cpp:1041
@@ +1040,3 @@
+  
+  uint8_t Buffer[9] = { dwarf::DW_CFA_GNU_args_size };
+  unsigned Len = encodeULEB128(Size, Buffer + 1) + 1;
----------------
DavidKreitzer wrote:
> 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?
> 
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()...


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2052
@@ +2051,3 @@
+      !MF.getTarget().getMCAsmInfo()->usesWindowsCFI() && 
+      (MF.getMMI().hasDebugInfo() || MF.getFunction()->needsUnwindTableEntry());
+
----------------
DavidKreitzer wrote:
> 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.
> 
Makes sense. I'll refine the condition.



http://reviews.llvm.org/D13132