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

[David Kreitzer via llvm-commits]

DavidKreitzer added inline comments.

================
Comment at: lib/MC/MCAsmStreamer.cpp:1038
@@ +1037,3 @@
+
+void MCAsmStreamer::EmitCFIGnuArgsSize(int64_t Size) {
+  MCStreamer::EmitCFIGnuArgsSize(Size);
----------------
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.)

================
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;
----------------
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?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2052
@@ +2051,3 @@
+      !MF.getTarget().getMCAsmInfo()->usesWindowsCFI() && 
+      (MF.getMMI().hasDebugInfo() || MF.getFunction()->needsUnwindTableEntry());
+
----------------
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.



http://reviews.llvm.org/D13132