[PATCH] D35844: Correct dwarf unwind information in function epilogue

[Violeta Vukobrat via Phabricator via llvm-commits]

violetav added inline comments.


================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:781
+  // block's entry. It is added to IncomingCFAOffset in CFIInfoVerifier.
+  int AdjustIncomingCFAOffset = 0;
+  // Amount used for adjusting the value of outgoing cfa offset. This value
----------------
iteratee wrote:
> I don't follow the comment about why these are necessary again.
> Can you elaborate?
I will try. So, let's say there is a function that looks like this:

    BB_0
      | 
    BB_1 <-----|
    /  \       |
  BB_2 BB_3    |
         |     |
       BB_4    |
         |     |
       BB_5 ___|

and that the following is true:

1. BB_1 has following cfi directives:

BB_1:
  ...
  .cfi_adjust_cfa_offset 4
  ...
  .cfi_adjust_cfa_offset -4
  ...

2. BB_4 has the same:

BB_4:
  ...
  .cfi_adjust_cfa_offset 4
  ...
  .cfi_adjust_cfa_offset -4
  ...

3. BB_1 is a successor of BB_5 (if it's not clear from the 'illustration', BB_0 has one successor, BB_1 has two, and BB_3,BB_4,BB_5 each have one).

4. For this example, first the cfi_adjust_cfa directives with positive values are inserted (in both BB_1 and BB_4) and then the ones with negative values.

What happens here with the previous approach is that, although BB_1 and BB_4 do not change cfa offset (their incoming and outgoing offsets are the same), they and their successors end up having the wrong offset set. 

This was the way that in/out offset was set to successors in the previous approach (from updateCFIInfoSucc()): 

  // the difference remains the same, calculate it by using previous in/out offset values
  int AdjustAmount = Succ->getOutgoingCFAOffset() - Succ->getIncomingCFAOffset();
  // set outgoing offset to be sum of the outgoing offset of the predecessor and the amount that this successor adjusts the offset itself (with adjust_cfa_directives)
  Succ->setOutgoingCFAOffset(CurrSucc->getOutgoingCFAOffset() + AdjustAmount);
  // set incoming offset to be the outgoing offset of the predecessor
  Succ->setIncomingCFAOffset(CurrSucc->getOutgoingCFAOffset());

Let's say that all blocks have in/out offset set to zero at the beginning (just to simplify the numbers).
When first 'adjust_cfa_offset 4' is inserted in BB_1, after updating the successors, the in/out cfa offsets of BBs in the function look like this:

BB_0, in 0, out 0
BB_1, in 0, out 4
BB_2, in 4, out 4
BB_3, in 4, out 4
BB_4, in 4, out 4
BB_5, in 4, out 4

Next, when 'adjust_cfa_offset 4' is inserted in BB_4:

BB_0, in 0, out 0
BB_1, in 8, out 12
BB_2, in 12, out 12
BB_3, in 12, out 12
BB_4, in 4, out 8
BB_5, in 8, out 8

Next, when 'adjust_cfa_offset -4' is inserted in BB_1:

BB_0, in 0, out 0
BB_1, in 8, out 8
BB_2, in 8, out 8
BB_3, in 8, out 8
BB_4, in 8, out 12
BB_5, in 12, out 12

Next, when 'adjust_cfa_offset -4' is inserted in BB_4:

BB_0, in 0, out 0
BB_1, in 8, out 8
BB_2, in 8, out 8
BB_3, in 8, out 8
BB_4, in 8, out 8
BB_5, in 8, out 8


And then you end up having wrong in/out cfa offset for blocks 1-5 because the adjustments are contained in OutgoingCFAOffset which is the value that gets propagated further. That is what is propagated to the successors, instead of the increment/decrement from adjust directives. This way of calculating in/out cfa offset values produces wrong results for the case where you have a cycle (like in the example above).


What is changed now is that each 'adjust_cfa_offset' directive is propagated to the successors when it's inserted and updates their incoming and outgoing adjust values appropriately. In the end, before verifying CFI info and inserting additional CFI instructions if needed, these values are added to incoming and outgoing cfa offset (that now contain only absolute offsets that are set).


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:772
 
+  // Verify basic block incoming and outgoing cfa offset and register values.
+  addPass(createCFIInfoVerifier());
----------------
iteratee wrote:
> I thought part of the problem was that these passes should not be run generally.
I put the added target hook in the passes themselves. Should I move it to TargetPassConfig?


Repository:
  rL LLVM

https://reviews.llvm.org/D35844