[PATCH] D37848: [ELF] - Dedupliсate FDEs correctly when two sections are ICFed

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In https://reviews.llvm.org/D37848#874114, @ruiu wrote:

> I don't think this is in the right direction. This seems too slow and too complicated. Why don't you just skip FDEs whose section IS does not satisfy `IS == IS->Repl`? Usually `IS == IS->Repl`, but if IS has been merged by ICF, `IS != IS->Repl`, so you can easily identify if a given section has been merged by ICF.


Honestly that was the first I thought of when started to work on the fix. I faced with breakage of eh-frame-hdr-icf.s that time.
Given its code:

  .globl _start, f1, f2
  _start:
    ret
  
  .section .text.f1, "ax"
  f1:
    .cfi_startproc
    ret
    .cfi_endproc
  
  .section .text.f2, "ax"
  f2:
    .cfi_startproc
    ret
    .cfi_endproc

ICF merged both `.text.f1` and `.text.f2` to `.text`. That way simple checking of `IS == IS->Repl` would just omit both two FDEs we have here.
I supposed that it is not correct and that we can leave single FDE and use its data then. Since FDE describes 
"how to unwind to the caller if the current instruction pointer is in the range covered by the FDE." I supposed it is fine to use it. 
I did not think that it can be ICF issue here. ICF stands for "identical code folding", I did not think about we should take FDE data into account
when doing ICF. 
Just like we do not support --icf=safe currently (and so far ICF is not safe anyways), I supposed we can assume that FDE data for identical code can be reused.

FWIW I just checked gold's behavior and it seems it is to use FDE of section that is alive after ICF. For case above it just drops both FDEs, but if we have 2
sections with different FDEs it uses the first FDE in order and ICF eliminates the second section. That also makes me think FDEs can be reused.

> That said, first of all, do you think you are doing is right? It seems that the current ICF code merges two different functions that are otherwise identical except FDEs.

We should take FDEs into account when comparing functions in ICF, shouldn't we? (I'm not saying that you should update this patch, but I want you to think.)

Given above I am not sure that we should do that. Taking FDEs into account is probably safest possible way to handle things, but I am not sure it is worth that as it may
slow down ICF and make its results to be worse.
If that path be chosen that seems we also need to check that their CIEs are the same as it should be possible to have different personality functions for example.


https://reviews.llvm.org/D37848