[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#875500, @ruiu wrote:

> You are saying that it is not correct to merge a .text section with other .text section if one has an FDE and the other doesn't.


No I did not :) What I was trying to say is that it is seems to me strange that we can merge such sections and lose valid FDE in result.
And so I *supposed* we should merge them but keep and use valid FDE. That is actually what code currently do. If we have 3 identical code sections,
and one of them does not have FDE, then all 3 be merged, and both 2 FDEs be placed to .eh_frame and one of FDEs will be filtered out
from .eh_frame_hdr finally.

> But you are also saying that we should merge two .text sections if both have FDEs even if their FDE contents are different. These two statements does not sound consistent to me. I don't think I'm convinced.

That is what we already do and it works. I can make synthetic testcase which will break it (can corrupt one of FDEs for example),
but synthetic case is probably not the good reason to change current behavior. Especially when we know that gold also just uses one of FDEs,
so behavior is probably acceptable.

> If you are saying that checking existence of FDEs is enough, can you argue why that is enough? If not, you could write code to demonstrate a problem (I guess you could write code that uses exceptions and behaves differently when two sections are merged by ICF) so that we can fix it. I don't think this patch fixes a real problem.

Patch fixes the real problem which is about redundant FDE entries LLD currently emits. We emit redundant duplicate entries in .eh_frame and have to adjust .eh_frame_hdr which also contains garbage data at its end.
So we have output larger it can be currently.

As mentioned earlier in previous comments, simpler approach can be used: just skip FDEs whose section IS does not satisfy `IS == IS->Repl`,
but I can prepare the sample to demonstrate the possible problem that can happen then.
It is also unfortunately a bit synthetic.
I tried to use -fno-asynchronous-unwind-tables flag to omit .eh_frame from output (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43232),
but it does not omit it for me. So what can be used to demonstrate the difference is next code:

  int test() {
    try {
      throw 1;
    }catch(...){}
   return 0;
  }

If I compile 2 files with the same test() and test1() to asm and drop .eh_frame from one of them manually. Then ICF will combine test() and test1().
Then depending on order of objects in inputs there will be either zero FDEs or single one in ouput I believe.

So my question is then - since testcase is synthetic, should we care about it ? 
If no then I think I agree we can just check `IS == IS->Repl` for now.


https://reviews.llvm.org/D37848