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

[Kyle Butt via llvm-commits]

On Thu, Aug 17, 2017 at 8:39 AM, Violeta Vukobrat via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>

Feel free to add asserts, but I think the answer needs to be that you can't
call updateCFIInfoSucc when the CFI state is inconsistent.
What is the correct thing to do propagate in this simpler case:
BB1:
if (cond) {
BB2:
  .cfi_adjust_cfa_offset 4
}
BB3:

Should you adjust BB3 or not?


>
> 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:
>

Except, you can't run the propagate algorithm at this point at all. There
is no correct answer, even if you did it from scratch. BB_1 has 2
predecessors with different cfa_offset


>
> 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:
>


Same, it's illegal to run the propogate algorithm here.


>
> 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:
>

Still illegal


>
> 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
>
>
Now that it's legal, you should propagate the results.
If you have a pass that's inserting directives in a way that you can't
propagate the results after you're done with a block, then you can keep a
list of modified blocks and propagate them when you're all finished, or
just recompute from scratch.


>
> 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).
>
>
The fact that the code you wrote previously didn't work right isn't a good
argument that you need the extra storage in every MBB.


>
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170817/1de5b1cb/attachment.html>