[PATCH] D13767: [X86] Fix more -Os + EH issues

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, Kevin!


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2108
@@ -2107,1 +2107,3 @@
 
+    // Similarly, if we don't have FP, but need to generate unwind information,
+    // we need to set the correct CFA offset at every call site.
----------------
kbsmith1 wrote:
> I think this code needs to go after line 2128 in order to create the CFI adjust after the stack adjustment instruction has happened rather than before.
Since to handle EH we only care about it being correct at the call site, I don't think it matters whether it's before or after the adjustment.

I could do it at line 2128, but that would be slightly less cumbersome, since then I'd need to keep track of both the original Amount and Amount - InternalAmt.


================
Comment at: test/CodeGen/X86/push-cfi-obj.ll:27
@@ +26,3 @@
+; LINUX-NEXT:      0020: 1C000000 24000000 00000000 19000000  |....$...........|
+; LINUX-NEXT:      0030: 04000000 00430E10 2E100020 4D0E1000  |.....C..... M...|
+; LINUX-NEXT:    )
----------------
kbsmith1 wrote:
> I don't understand what the specific differences in these values in Section Data coma about from.  Do you know?
More or less. :-)

I mean, the part that *didn't* change encodes the GNU_ARGS_SIZE, and this is what this test is supposed to check (it was introduced by a previous commit that made some changes to the code that outputs the dwarf binary encoding).
The newly added part encodes the def_cfa_offset. I didn't actually verify it's correct because I didn't change the encoding code for that. I'll parse it manually and see it really is correct.


================
Comment at: test/CodeGen/X86/push-cfi.ll:14
@@ +13,3 @@
+; LINUX: .cfi_escape 0x2e, 0x10
+; LINUX: .cfi_adjust_cfa_offset 16
+; LINUX-NEXT: pushl   $4
----------------
kbsmith1 wrote:
> I don't think this is really the correct spot for the cfi_adjust.  I think it should occur after all the pushes have happenned, so right before the call instruction. Once you get to having a different cfi_adjust per push, each of these should follow immediately after the instruction, not before.  Now I understand that you are not yet getting to the point of emitting a cfi_adjust per instruction, but it seems like even if you are accumulating them so that they are only correct at the call, then they should be after all the pushes.
I agree with you that it's not the correct spot, in theory - but in terms of being correct at the call site, it doesn't matter if it's before or after the pushes.

The problem is that the way things work right now, eliminateCallFramePseudoInstr() doesn't actually get a pointer to the call, it gets the ADJDOWN/ADJUP pseudos. In theory, there doesn't even *have* to be a call between them. 
So, the easiest way is to just put the CFA adjust immediately after those pseudos. I could have the eliminate() code actively look for a call, but I'm not sure that's the best idea. What I would prefer for the debug-info version would be to put the CFA adjust for the sub where the current adjust is (well, after the sub - in line 2128), and the adjust for each push after the push. So that case wouldn't have to search for a call either.


http://reviews.llvm.org/D13767